refactor(SaneQL): map pull up: pull filter nodes across map nodes - #1344
Draft
fengelniederhammer wants to merge 6 commits into
Draft
refactor(SaneQL): map pull up: pull filter nodes across map nodes#1344fengelniederhammer wants to merge 6 commits into
fengelniederhammer wants to merge 6 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Implements the #1343 prerequisite (Expression::freeIUs() coverage for filter predicates) and uses it to safely move filters across MapNode during optimization, avoiding semantic changes when predicates depend on Map-produced columns (notably decompression maps). Also teaches ColumnNarrowingPass to retain Map assignments needed by filter predicates and adds regression tests.
Changes:
- Add
FilterPushdownPasshandling forMapNode, splitting dependent vs. independent filters usingfreeIUs(). - Implement
freeIUs()for essentially all filter predicate expression types and composite predicates (And/Or/NOf/wrappers). - Add/update optimizer tests covering “do not push through Map when predicate depends on produced column” and “keep Map assignment required by filter”.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/silo/query_engine/optimizer/filter_pushdown_pass.test.cpp | Adds Map-dependent filter pushdown regression tests |
| src/silo/query_engine/optimizer/filter_pushdown_pass.h | Adds MapNode visitor overload declaration |
| src/silo/query_engine/optimizer/filter_pushdown_pass.cpp | Implements Map-aware filter pushdown splitting/merging |
| src/silo/query_engine/optimizer/column_narrowing_pass.test.cpp | Adds regression test for keeping Map assignment used by filter |
| src/silo/query_engine/optimizer/column_narrowing_pass.h | Declares FilterNode visitor overload |
| src/silo/query_engine/optimizer/column_narrowing_pass.cpp | Adds FilterNode handling to include predicate refs in required |
| src/silo/query_engine/expressions/symbol_in_set.h | Adds freeIUs() for sequence symbol-set predicate |
| src/silo/query_engine/expressions/symbol_equals.h | Adds freeIUs() for sequence symbol-equals predicate |
| src/silo/query_engine/expressions/string_search.h | Declares freeIUs() for string regex predicate |
| src/silo/query_engine/expressions/string_search.cpp | Implements freeIUs() for string regex predicate |
| src/silo/query_engine/expressions/string_in_set.h | Declares freeIUs() for string-in-set predicate |
| src/silo/query_engine/expressions/string_in_set.cpp | Implements freeIUs() for string-in-set predicate |
| src/silo/query_engine/expressions/string_equals.h | Declares freeIUs() for string-equals predicate |
| src/silo/query_engine/expressions/string_equals.cpp | Implements freeIUs() for string-equals predicate |
| src/silo/query_engine/expressions/phylo_child_filter.h | Declares freeIUs() for phylo predicate |
| src/silo/query_engine/expressions/phylo_child_filter.cpp | Implements freeIUs() for phylo predicate |
| src/silo/query_engine/expressions/or.h | Declares freeIUs() for Or composite predicate |
| src/silo/query_engine/expressions/or.cpp | Implements freeIUs() for Or composite predicate |
| src/silo/query_engine/expressions/nof.h | Declares freeIUs() for NOf composite predicate |
| src/silo/query_engine/expressions/nof.cpp | Implements freeIUs() for NOf composite predicate |
| src/silo/query_engine/expressions/negation.h | Declares freeIUs() for Negation wrapper predicate |
| src/silo/query_engine/expressions/negation.cpp | Implements freeIUs() for Negation wrapper predicate |
| src/silo/query_engine/expressions/mutation_profile.h | Adds freeIUs() for mutation-profile predicate |
| src/silo/query_engine/expressions/maybe.h | Declares freeIUs() for Maybe wrapper predicate |
| src/silo/query_engine/expressions/maybe.cpp | Implements freeIUs() for Maybe wrapper predicate |
| src/silo/query_engine/expressions/lineage_filter.h | Declares freeIUs() for lineage predicate |
| src/silo/query_engine/expressions/lineage_filter.cpp | Implements freeIUs() for lineage predicate |
| src/silo/query_engine/expressions/is_null.h | Declares freeIUs() for is-null predicate |
| src/silo/query_engine/expressions/is_null.cpp | Implements freeIUs() for is-null predicate |
| src/silo/query_engine/expressions/int_equals.h | Declares freeIUs() for int-equals predicate |
| src/silo/query_engine/expressions/int_equals.cpp | Implements freeIUs() for int-equals predicate |
| src/silo/query_engine/expressions/int_between.h | Declares freeIUs() for int-between predicate |
| src/silo/query_engine/expressions/int_between.cpp | Implements freeIUs() for int-between predicate |
| src/silo/query_engine/expressions/insertion_contains.h | Adds freeIUs() for insertion predicate |
| src/silo/query_engine/expressions/has_mutation.h | Adds freeIUs() for has-mutation predicate |
| src/silo/query_engine/expressions/float_equals.h | Declares freeIUs() for float-equals predicate |
| src/silo/query_engine/expressions/float_equals.cpp | Implements freeIUs() for float-equals predicate |
| src/silo/query_engine/expressions/float_between.h | Declares freeIUs() for float-between predicate |
| src/silo/query_engine/expressions/float_between.cpp | Implements freeIUs() for float-between predicate |
| src/silo/query_engine/expressions/expression.h | Updates freeIUs docs + adds collectFreeIUs helper |
| src/silo/query_engine/expressions/exact.h | Declares freeIUs() for Exact wrapper predicate |
| src/silo/query_engine/expressions/exact.cpp | Implements freeIUs() for Exact wrapper predicate |
| src/silo/query_engine/expressions/date_equals.h | Declares freeIUs() for date-equals predicate |
| src/silo/query_engine/expressions/date_equals.cpp | Implements freeIUs() for date-equals predicate |
| src/silo/query_engine/expressions/date_between.h | Declares freeIUs() for date-between predicate |
| src/silo/query_engine/expressions/date_between.cpp | Implements freeIUs() for date-between predicate |
| src/silo/query_engine/expressions/bool_equals.h | Declares freeIUs() for bool-equals predicate |
| src/silo/query_engine/expressions/bool_equals.cpp | Implements freeIUs() for bool-equals predicate |
| src/silo/query_engine/expressions/and.h | Declares freeIUs() for And composite predicate |
| src/silo/query_engine/expressions/and.cpp | Implements freeIUs() for And composite predicate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fengelniederhammer
force-pushed
the
1258-filter-map-pullup
branch
from
July 1, 2026 09:28
282f504 to
86dc983
Compare
Comment on lines
+167
to
+174
| for (const auto& referenced_column : node.filter->freeIUs()) { | ||
| const bool already_required = std::ranges::any_of(required, [&](const auto& existing) { | ||
| return existing.name == referenced_column.name; | ||
| }); | ||
| if (!already_required) { | ||
| required.push_back(referenced_column); | ||
| } | ||
| } |
Comment on lines
+137
to
+148
| std::vector<schema::ColumnIdentifier> result; | ||
| for (const auto& expr : expressions) { | ||
| for (const auto& col : expr->freeIUs()) { | ||
| const bool already_present = std::ranges::any_of(result, [&](const auto& existing) { | ||
| return existing.name == col.name; | ||
| }); | ||
| if (!already_present) { | ||
| result.push_back(col); | ||
| } | ||
| } | ||
| } | ||
| return result; |
| value(std::move(value)) {} | ||
|
|
||
| std::vector<schema::ColumnIdentifier> StringEquals::freeIUs() const { | ||
| return {{.name = column_name, .type = schema::ColumnType::BOOL}}; |
Contributor
Author
There was a problem hiding this comment.
Is there actually a way to figure out the correct column type? It can be STRING or INDEXED_STRING here. isNull has a similar issue.
fengelniederhammer
force-pushed
the
1258-filter-map-pullup
branch
from
July 15, 2026 06:13
86dc983 to
c1c795a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
resolves #1343
Summary
Fixes a correctness bug where a filter on a decompressed column produced wrong results, by teaching the optimizer which columns a filter predicate references.
The auto-inserted decompression
MapNodeproduces aSTRINGcolumn with the same name as its compressed source (e.g.seq→seq).FilterPushdownPasspushed every filter down pastMapNodes, sodefault.filter(seq = 'ACGT')ran the filter against the still-compressed column.Changes:
Expression::freeIUs()on all predicate types (previously onlyFieldRef/At/ZstdDecompressScalar). It reports the columns a predicate references. For filter predicates only.nameis reliable;.typeis a placeholder, except sequence predicates which carry their sequence type.FilterPushdownPassno longer pushes a filter below aMapthat produces a column the filter consumes. Such filters stay above theMap; independent filters still push down. Sequence predicates (e.g.hasMutation) consume the Map's sequence-typed input, so they keep pushing down to the scan.ColumnNarrowingPassnow keepsMapassignments alive when a surviving filter above theMapreferences the produced column.Pass order is unchanged (
ColumnNarrowing → FilterPushdown → MapPullup → NodeResolution).PR Checklist
- [ ] All necessary documentation has been adapted or there is an issue to do so.