Enable file pruning on nested columns#820
Conversation
Revert "Add primitive support for conjunction filters" This reverts commit 9584122. This reverts commit c31e003. Better implementation in duckdb#820
|
Changes look great on the outset, but this needs some tests still |
894940f to
18df3a4
Compare
| ---- | ||
| 1 Alice {'street': 123 Main St, 'city': Metropolis, 'zip': 12345} [123-456-7890, 987-654-3210] {age=30, membership=gold} | ||
|
|
||
| # nested-column filters prune files from scan |
There was a problem hiding this comment.
Thanks for the tests, can we extend this to cover:
- multiple types
- deeper nesting
- lists
- maps
- IS NULL / IS NOT NULL on child columns
- IS NULL / IS NOT NULL on structs/lists/maps (not elements)
- conjunction AND on combinations of the above items
There was a problem hiding this comment.
I've added a few new tests in test/sql/local/irc_any_catalog/reads/test_nested_column_pruning.test. Files are not currently pruned when filtering on elements of lists or maps, as PushDownFilterIntoExpr() leaves these as generic ExpressionFilters, nor with IS NULL or IS NOT NULL predicates, as FilterCombiner::TryPushdownExpression() does not treat them.
Note that many of these demonstrate cases where filter pushdown does _not_ work.
IcebergPredicateStats::DeserializeBounds refuses to set null bounds, making it impossible to distinguish between columns where the bounds are unset and columns that contain only null. Use has_not_null to disambiguate.
fixes lakekeeper test
|
Ahoy, is there anything else that needs to happen before this can be merged? |
|
I'm very sorry to ask this, but the alternative is to close your PR: can you rebase with updated There's been a decent sized shift upstream (in |
|
Thanks for coming back to this! I see that a lot has changed under the feet of this PR, which is just what happens when one lives for too long. I will check to what degree its approach is still relevant/feasible/advisable and then rebase or close as appropriate. |
|
@Tishj After digging around the current Is there a reason not to do this? Otherwise I would try to implement it. |
|
I think you're right, and I don't think there's a reason not to do this, it's just not implemented yet |
After thinking more carefully about what I was trying to achieve in #766, I realized that most of my underlying problem was related to the way
FilterCombinertreats nested columns. Given astruct_extractexpression, it wraps the inner filter in aStructFilterand pushes it into the filter set as a filter on the containing column. This has two unhelpful side-effects:ConjunctionAndFilter.This PR attempts to address both of these by inverting the combination and wrapping that
FilterCombinerdoes to create a decomposedTableFilterSetthat refers to individual columns. This is then used both for partition and range pruning inFileMatchesFilter().I suspect it would have been cleaner to apply this transformation once to
IcebergMultiFileList::table_filtersrather than for every single data file, but there appears to be some tight coupling betweenIcebergMultiFileList::table_filtersandIcebergMultiFileReaderthat I can't quite wrap my head around yet. Suggestions welcome!