opt: add rule to fold limits#49931
Conversation
|
This change is |
andy-kimball
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/sql/opt/norm/rules/limit.opt, line 160 at r1 (raw file):
[FoldLimits, Normalize] (Limit $input:(Limit
NIT: For consistency, I'd make this $outerInput.
pkg/sql/opt/norm/testdata/rules/limit, line 50 at r1 (raw file):
└── 100 # Don't eliminate the outer limit if it's less than the inner.
NIT: Should update this comment to be more clear, since outer limit is in fact "eliminated" (just not by the EliminateLimit rule).
pkg/sql/opt/norm/testdata/rules/limit, line 88 at r1 (raw file):
# Don't eliminate in case of negative limit. norm expect-not=EliminateLimit
NIT: Similar comment issue here.
pkg/sql/opt/norm/testdata/rules/limit, line 1113 at r1 (raw file):
└── 5 # Case where the outer limit ordering implies the inner ordering.
I don't think this is the most interesting case, because b is functionally dependent on a. I'd add a different case, where the ordered columns are independent of one another. Actually, two different cases, where the inner and outer orderings are swapped.
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/limit.opt, line 160 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
NIT: For consistency, I'd make this $outerInput.
Actually, I just realized that doesn't have to be bound at all. Done.
pkg/sql/opt/norm/testdata/rules/limit, line 50 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
NIT: Should update this comment to be more clear, since outer limit is in fact "eliminated" (just not by the
EliminateLimitrule).
Done.
pkg/sql/opt/norm/testdata/rules/limit, line 88 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
NIT: Similar comment issue here.
Done.
pkg/sql/opt/norm/testdata/rules/limit, line 1113 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
I don't think this is the most interesting case, because
bis functionally dependent ona. I'd add a different case, where the ordered columns are independent of one another. Actually, two different cases, where the inner and outer orderings are swapped.
Done.
dabd252 to
bdf711c
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, 3 of 3 files at r2.
Reviewable status:
pkg/sql/opt/norm/rules/limit.opt, line 208 at r2 (raw file):
# FoldLimits replaces a Limit on top of a Limit with a single Limit operator # when the outer limit value is smaller than or equal to the inner limit value # and the orderings intersect.
[nit] might be worth mentioning that the opposite case, when the outer limit is larger than the inner limit, is already handled by the EliminateLimit rule.
pkg/sql/opt/norm/testdata/rules/limit, line 1322 at r2 (raw file):
└── 5 # No-op case where the outer limit is larger than the inner limit.
(add a note that the limit is removed by EliminateLimit)
pkg/sql/opt/xform/testdata/rules/limit, line 65 at r2 (raw file):
│ └── scan a@s_idx │ ├── columns: i:2 s:4 │ ├── limit: 10
Hmm this seems suboptimal that we're removing the limit in this case. Is there a way to avoid this?
bdf711c to
d53b728
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/limit.opt, line 208 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] might be worth mentioning that the opposite case, when the outer limit is larger than the inner limit, is already handled by the
EliminateLimitrule.
Done.
pkg/sql/opt/norm/testdata/rules/limit, line 1322 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
(add a note that the limit is removed by EliminateLimit)
Done.
pkg/sql/opt/xform/testdata/rules/limit, line 65 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Hmm this seems suboptimal that we're removing the limit in this case. Is there a way to avoid this?
Interesting, I didn't notice that. I'm not sure it could be done as a norm rule; this might suggest that this should be an exploration rule. Or we could only match when the inner ordering implies the outer ordering.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3.
Reviewable status:
pkg/sql/opt/norm/rules/limit.opt, line 209 at r3 (raw file):
# when the outer limit value is smaller than or equal to the inner limit value # and the orderings intersect. Note: the case when the outer limit value is # larger than the smaller is handled by EliminateLimit.
larger than the smaller -> larger than the inner
pkg/sql/opt/xform/testdata/rules/limit, line 65 at r2 (raw file):
Or we could only match when the inner ordering implies the outer ordering.
This sounds like a good solution to me. I think if we can avoid adding unnecessary exploration rules that's a good thing.
d53b728 to
53d3a0a
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/rules/limit.opt, line 209 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
larger than the smaller -> larger than the inner
Done.
pkg/sql/opt/xform/testdata/rules/limit, line 65 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Or we could only match when the inner ordering implies the outer ordering.
This sounds like a good solution to me. I think if we can avoid adding unnecessary exploration rules that's a good thing.
Done.
53d3a0a to
11c226d
Compare
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r4.
Reviewable status:
pkg/sql/opt/norm/testdata/rules/limit, line 1276 at r3 (raw file):
└── 5 # Case where the outer limit ordering implies the inner ordering.
I'd leave this test case, but use expect-not.
Previously, there was no rule to fold a Limit on top of a Limit when the outer limit value is smaller than the inner limit value. This patch adds a rule to fold two Limits together when the outer Limit has a smaller limit value than the inner Limit and the inner ordering implies the outer ordering. Release note (sql change): The optimizer can now fold two Limit operators together.
11c226d to
759e144
Compare
DrewKimball
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/testdata/rules/limit, line 1276 at r3 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
I'd leave this test case, but use expect-not.
Done.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status:
rytaft
left a comment
There was a problem hiding this comment.
Reviewable status:
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/testdata/rules/limit, line 65 at r2 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
Done.
I agree!
|
bors r+ |
Build succeeded |
Previously, there was no rule to fold a Limit on top of a Limit
when the outer limit value is smaller than the inner limit value.
This patch adds a rule to fold two Limits together when the
outer Limit has a smaller limit value than the inner Limit and
the inner ordering implies the outer ordering.
Release note (sql change): The optimizer can now fold two Limit
operators together.