Skip to content

Added parquet support for filters parameter#1308

Open
gtorrini wants to merge 1 commit into
lsst:mainfrom
gtorrini:parquet_filtering
Open

Added parquet support for filters parameter#1308
gtorrini wants to merge 1 commit into
lsst:mainfrom
gtorrini:parquet_filtering

Conversation

@gtorrini

@gtorrini gtorrini commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes
  • (if changing dimensions.yaml) make a copy of dimensions.yaml in configs/old_dimensions

Notes

  • Missing release note (no ticket associated yet).
  • I didn't do any semantic validation on the filters, as this is performed by filters_to_expression early on in ParquetDataset initialization.
  • I tried to get started on some basic unit test checks, but it seems there's a wrapper/except block preventing the ValueError for invalid columns in the filter predicates from being interpreted directly. See the #FIXME comments in test_parquet.py. The get call outside the assertRaises context manager DOES raise a ValueError, but the get call inside the assertRaises context manager raises an AssertionError for not raising a value error.
====================================== short test summary info =======================================
FAILED test_parquet.py::ParquetFormatterDataFrameTestCase::testMultiIndexDataFrame - ValueError: Failure from formatter 'lsst.daf.butler.formatters.parquet.ParquetFormatter' for data...
FAILED test_parquet.py::InMemoryDataFrameDelegateTestCase::testSingleIndexDataFrame - AssertionError: ValueError not raised

@codecov

codecov Bot commented Dec 11, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 20.00000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.46%. Comparing base (5a7fd38) to head (1280ff0).
⚠️ Report is 186 commits behind head on main.

Files with missing lines Patch % Lines
python/lsst/daf/butler/formatters/parquet.py 20.00% 11 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1308      +/-   ##
==========================================
- Coverage   89.48%   89.46%   -0.03%     
==========================================
  Files         367      367              
  Lines       49852    49867      +15     
  Branches     5855     5863       +8     
==========================================
+ Hits        44611    44614       +3     
- Misses       3853     3864      +11     
- Partials     1388     1389       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@gtorrini
gtorrini marked this pull request as draft December 11, 2025 23:01
@gtorrini
gtorrini marked this pull request as ready for review December 12, 2025 00:57
@timj
timj requested a review from erykoff December 12, 2025 20:29
if col not in schema.names:
raise ValueError(
f"Column {col} specified in filters not available in parquet file."
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the indentation correct above? This won't do the check if it is an Expression; is that intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not, I should fix it to perform the check if it is an Expression as well.

if all(isinstance(item, tuple) for item in par_filters):
par_filters = [par_filters] # convert to DNF for column checking
elif not all(isinstance(item, list) for item in par_filters):
raise TypeError("List filters must contain tuples or lists of tuples.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this section at all. If it's a list of tuples you convert a list to a list of lists? And then if it's a list of lists it raises?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted the list of tuples to a list of list of tuples for streamlined column checking later on (par_filters in DNF). It should only raise the TypeError if it is not a list of tuples or list of lists.

Comment thread tests/test_parquet.py
# but Ctrl+f "ValueErrorWeirdness2" below.
# with self.assertRaises(ValueError):
# self.butler.get(self.datasetType, dataId={},
# parameters={"filters": [("e", ">", 1)]})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests of the functionality is missing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is addressed in the third bullet points of the notes above. I couldn't figure out why it raises a ValueError outside of the context manager, but calling the context manager to check for a ValueError raises an AssertionError.

@gtorrini

Copy link
Copy Markdown
Contributor Author

After revisiting Eli's feedback and doing some more research, I think implementing parquet filter support is probably above my pay grade:

Passing filters without any field checks to pq.read_table--even if they are a properly formatted ds.Expression, List[Tuple], or List[List[Tuple]]--could result in some very late errors raised by PyArrow. I'm not sure what degree of validation is preferred at the daf_butler API boundary, so I'll defer to someone with more expertise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants