-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
opt: add FD equivalency for semi-passthrough projection columns #43465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, but maybe add a props test case
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, @RaduBerinde, and @rytaft)
pkg/sql/opt/memo/logical_props_builder.go, line 272 at r1 (raw file):
} } // Also add any column that is a direct reference to a non-null column. The
I'd add a test for this new case into memo/testdata/logprops/project
.
In certain cases, we have synthesized projections that are simple variable expressions (I hereby name these semi-passthrough). This can happen for example if a non-trivial expression gets constant folded to a single variable (perhaps after placeholder replacement). The optimizer can also construct these to assign different ColumnIDs. This change adds the equivalency between the semi-passthrough column and the one it refers to, and also uses it to mark columns as non-null. This improves the plan when we are ordering on such a semi-passthrough column (as long as both columns are projected). There will be a follow-up fix that makes the `CanProvideOrdering` logic better to handle this case even when the original column is not projected. Fixes cockroachdb#43360.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR!
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball, @justinj, and @rytaft)
pkg/sql/opt/memo/logical_props_builder.go, line 272 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
I'd add a test for this new case into
memo/testdata/logprops/project
.
Done. Added a test for the null column and one for the FD.
bors r+ |
Hm, I don't think I pushed that last update bors r- |
Canceled |
03aa4e1
to
6377847
Compare
bors r+ |
43465: opt: add FD equivalency for semi-passthrough projection columns r=RaduBerinde a=RaduBerinde In certain cases, we have synthesized projections that are simple variable expressions (I hereby name these semi-passthrough). This can happen for example if a non-trivial expression gets constant folded to a single variable (perhaps after placeholder replacement). The optimizer can also construct these to assign different ColumnIDs. This change adds the equivalency between the semi-passthrough column and the one it refers to, and also uses it to mark columns as non-null. This improves the plan when we are ordering on such a semi-passthrough column (as long as both columns are projected). There will be a follow-up fix that makes the `CanProvideOrdering` logic better to handle this case even when the original column is not projected. Fixes #43360. Co-authored-by: Radu Berinde <radu@cockroachlabs.com>
Build succeeded |
Release note (performance improvement): the optimizer generates better execution plans in some cases where there is an ORDER BY expression that simplifies to a simple variable reference. |
In certain cases, we have synthesized projections that are simple
variable expressions (I hereby name these semi-passthrough). This can
happen for example if a non-trivial expression gets constant folded to
a single variable (perhaps after placeholder replacement). The
optimizer can also construct these to assign different ColumnIDs.
This change adds the equivalency between the semi-passthrough column
and the one it refers to, and also uses it to mark columns as
non-null. This improves the plan when we are ordering on such a
semi-passthrough column (as long as both columns are
projected). There will be a follow-up fix that makes the
CanProvideOrdering
logic better to handle this case even when theoriginal column is not projected.
Fixes #43360.