Remove duplicated WhereConstraintNodes in predicate pushdown #1304
Conversation
This file contains 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
This was referenced Jun 25, 2024
tlento
changed the title
tlento
commented
Jun 25, 2024
| @@ -48,71 +46,55 @@ FROM ( | |||
| ON | |||
| subq_36.listing = listings_latest_src_28000.listing_id | |||
| ) subq_41 | |||
| WHERE (listing__is_lux_latest) AND (booking__is_instant) | |||
| WHERE listing__is_lux_latest | |||
There was a problem hiding this comment.
Note - this is an example of removing part of a filter expression - the booking__is_instant filter was applied via pushdown (in the collapsed code section above) on the left side of the join, and we no longer repeat it here.
tlento
added
the
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
github-actions
bot
removed
the
Run Tests With Other SQL Engines
tlento
force-pushed
the
remove-pushdown-duplicated-where-constraints
branch
2 times, most recently
from
3aaca4b to
3c87606
Compare
tlento
added
the
Run Tests With Other SQL Engines
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
tlento
temporarily deployed
to
DW_INTEGRATION_TESTS
GitHub Actions
Inactive
This was referenced Jun 25, 2024
github-actions
bot
removed
the
Run Tests With Other SQL Engines
courtneyholcomb
approved these changes
Jun 25, 2024
| @@ -48,71 +46,55 @@ FROM ( | |||
| ON | |||
| subq_36.listing = listings_latest_src_28000.listing_id | |||
| ) subq_41 | |||
| WHERE (listing__is_lux_latest) AND (booking__is_instant) | |||
| WHERE listing__is_lux_latest | |||
tlento
force-pushed
the
06-24-add_direct_tests_for_predicate_pushdown_optimizer
branch
from
cb955d2 to
53767dd
Compare
tlento
force-pushed
the
remove-pushdown-duplicated-where-constraints
branch
from
3c87606 to
981595e
Compare
tlento
force-pushed
the
06-24-add_direct_tests_for_predicate_pushdown_optimizer
branch
from
53767dd to
eeb1068
Compare
Base automatically changed from
06-24-add_direct_tests_for_predicate_pushdown_optimizer
to
main
June 26, 2024 00:39
The original implementation of predicate pushdown generated duplicated where filter constraints, applying it once as a result of pushdown and again in the original where constraint node. This change removes the duplication, and does so at the level of the individual filter spec. Logically, this means the following: 1. If all filter specs for a where constraint node can be pushed down, the node itself will be moved past the join. 2. If some filter specs for a where constraint node can be pushed down, the node itself will remain in place, but it will only apply the filters that cannot be pushed down. 3. Any WhereConstraintNode added to the DataflowPlan for the purposes of cleaning up spurious rows added as the result of an outer join, such as we have in one particular JoinToTimeSpineNode scenario, must be annotated to indicate that we should apply that constraint filter no matter what. This change updates all of our logic to ensure that we effectively apply each where filter exactly once within a branch in the DAG, and only re-apply filters where explicitly requested at build time. Note this change is all based on certain assumptions about how the DataflowPlanBuilder constructs the plan. Specifically, we assume that we will never need to re-apply filters upstream of a node unless otherwise indicated. If we start to do fancy things with nesting where constraints or swapping join sides - particularly if we push groupable metric filters to an inner join or if we begin adding query time constraints outside of an aggregated output join - this could get dodgy. Conversion metrics are of particular concern here.
Due to an oversight the functionally_identical and with_new_parents logic was not updated on the WhereConstraintNode to account for the always_apply property. In isolation this doesn't affect the predicate pushdown optimizer, but in conjunction with the source scan optimizer this causes problems. Rather than merge something broken I'm amending this PR.
tlento
force-pushed
the
remove-pushdown-duplicated-where-constraints
branch
from
981595e to
bb4eda4
Compare
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.
The original implementation of predicate pushdown generated duplicated
where filter constraints, applying it once as a result of pushdown and
again in the original where constraint node.
This change removes the duplication, and does so at the level of the
individual filter spec. Logically, this means the following:
the node itself will be moved past the join.
the node itself will remain in place, but it will only apply the filters
that cannot be pushed down.
cleaning up spurious rows added as the result of an outer join, such as
we have in one particular JoinToTimeSpineNode scenario, must be annotated
to indicate that we should apply that constraint filter no matter what.
This change updates all of our logic to ensure that we effectively apply
each where filter exactly once within a branch in the DAG, and only re-apply
filters where explicitly requested at build time.
Note this change is all based on certain assumptions about how the
DataflowPlanBuilder constructs the plan. Specifically, we assume that we will
never need to re-apply filters upstream of a node unless otherwise
indicated. If we start to do fancy things with nesting where constraints
or swapping join sides - particularly if we push groupable metric filters
to an inner join or if we begin adding query time constraints outside of
an aggregated output join - this could get dodgy. Conversion metrics are
of particular concern here.