opt: improve test coverage and remove unused rules#29050
opt: improve test coverage and remove unused rules#29050craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
andy-kimball
left a comment
There was a problem hiding this comment.
checkExpr.
I like the directive idea. I think we probably have a lot of tests that are no longer testing what they were intended to test, and a directive would nicely solve this problem.
Reviewable status:
complete! 0 of 0 LGTMs obtained (and 1 stale)
pkg/sql/opt/norm/rules/join.opt, line 27 at r1 (raw file):
# not already exist. This allows other rules to rely upon the presence of the # Filters when matching. See comment at top of bool.opt for more details. [EnsureJoinFilters, Normalize]
I think it's OK to remove this rule (as well EnsureSelectFiltersAnd). However, can you add a new check to Factory.checkExpr to validate that every Join and Select with a non-trivial filter are using Filters? That would prevent us from forgetting to construct it.
|
PTAL - I ended up having to modify things a bit because |
Brought up in cockroachdb#29050 as a possibility to avoid the case of certain rules no longer applying. Kind of a strawman right now because I'm not sure * if this is the syntax we want, * how widely we want to use it, I've just added it to one file for the purposes of this PR. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 8 of 8 files at r1, 3 of 3 files at r2.
Reviewable status:
pkg/sql/opt/norm/check_expr.go, line 89 at r2 (raw file):
filter := ev.Child(1) switch filter.Operator() { case opt.FiltersOp, opt.TrueOp:
Isn't it possible for Select to have a filter with FalseOp? (TrueOp should have been normalized away, I think...)
pkg/sql/opt/norm/testdata/rules/bool, line 461 at r2 (raw file):
# -------------------------------------------------- opt SELECT * FROM c WHERE NOT(NOT(a))
Why did you need to change this test?
pkg/sql/opt/norm/testdata/rules/ordering, line 167 at r2 (raw file):
# Remove column functionally dependent on multi-column key. opt SELECT * FROM (SELECT * FROM abcde WHERE b IS NOT NULL AND c IS NOT NULL ORDER BY c, d, b, e) WITH ORDINALITY
Why did you need to change this test?
Brought up in cockroachdb#29050 as a possibility to avoid the case of certain rules no longer applying. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None
Brought up in cockroachdb#29050 as a possibility to avoid the case of certain rules no longer applying. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None
29135: opt: directive that asserts a rule was used r=justinj a=justinj Brought up in #29050 as a possibility to avoid the case of certain rules no longer applying. Kind of a strawman right now because I'm not sure * if this is the syntax we want, * how widely we want to use it, I've just added it to one file for the purposes of this PR. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None 29219: kv: truncate RangeFeed span to range descriptor r=nvanbenschoten a=nvanbenschoten Small fix for RangeFeed's interaction with DistSender. Co-authored-by: Justin Jaffray <justin@cockroachlabs.com> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
justinj
left a comment
There was a problem hiding this comment.
Sorry, thought I responded to this one!
Reviewable status:
pkg/sql/opt/norm/check_expr.go, line 89 at r2 (raw file):
Previously, rytaft wrote…
Isn't it possible for
Selectto have a filter withFalseOp? (TrueOpshould have been normalized away, I think...)
Hm, for some reason I thought TrueOp was a possible value here, but removing it doesn't trigger this, so I guess I was mistaken. FalseOp also gets normalized away (by DetectSelectContradiction I think).
pkg/sql/opt/norm/rules/join.opt, line 27 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
I think it's OK to remove this rule (as well
EnsureSelectFiltersAnd). However, can you add a new check toFactory.checkExprto validate that everyJoinandSelectwith a non-trivial filter are usingFilters? That would prevent us from forgetting to construct it.
Done.
pkg/sql/opt/norm/testdata/rules/bool, line 461 at r2 (raw file):
Previously, rytaft wrote…
Why did you need to change this test?
Before it wasn't actually using EliminateNot - it was going NOT(NOT(j = '{}')) -> NOT(j != '{}') -> j = '{}'
pkg/sql/opt/norm/testdata/rules/ordering, line 167 at r2 (raw file):
Previously, rytaft wrote…
Why did you need to change this test?
The WITH ORDINALITY was at the wrong level before, so it wasn't exercising the rule it claimed to.
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:
pkg/sql/opt/norm/check_expr.go, line 89 at r2 (raw file):
Previously, justinj (Justin Jaffray) wrote…
Hm, for some reason I thought
TrueOpwas a possible value here, but removing it doesn't trigger this, so I guess I was mistaken.FalseOpalso gets normalized away (byDetectSelectContradictionI think).
Ah right - cool.
EliminateEmpty{And,Or} were superseded by SimplifyAnd and SimplifyOr.
Ensure{Join,Select}FiltersAnd were superseded by SimplifyFilters.
I'm not sure about removing EnsureJoinFilters - I can't see a way to
trigger it since I think we're generally pretty careful about
constructing Joins with Filters, but its removal does increase the
probability that we'll create an invalid Join in the future (possibly
without noticing).
Also change tests so that SimplifyRowNumberOrdering and EliminateNot
trigger now.
Release note: None
|
TFTRs! bors r+ |
29050: opt: improve test coverage and remove unused rules r=justinj a=justinj
EliminateEmpty{And,Or} were superseded by SimplifyAnd and SimplifyOr.
Ensure{Join,Select}FiltersAnd were superseded by SimplifyFilters.
I'm not sure about removing EnsureJoinFilters - I can't see a way to
trigger it since I think we're generally pretty careful about
constructing Joins with Filters, but its removal does increase the
probability that we'll create an invalid Join in the future (possibly
without noticing).
Also change tests so that SimplifyRowNumberOrdering and EliminateNot
trigger now.
One thought I have is that we have lots of these clearly demarcated tests that are intended to test a particular rule - perhaps we should consider introducing a directive which fails a test if a certain set of rules aren't triggered.
Release note: None
Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Build succeeded |
Brought up in cockroachdb#29050 as a possibility to avoid the case of certain rules no longer applying. I think this is probably valuable in that it asserts we're testing what we want to test, but I do think it's a little invasive as far as cluttering up the test directives goes. Open to suggestions for how to possibly improve. Release note: None
|
@justinj this was never backported and I'm running into some merge conflicts with another backport. This doesn't backport cleanly anymore, it needs some manual fixing.. Can you take care of it ASAP? |
EliminateEmpty{And,Or} were superseded by SimplifyAnd and SimplifyOr.
Ensure{Join,Select}FiltersAnd were superseded by SimplifyFilters.
I'm not sure about removing EnsureJoinFilters - I can't see a way to
trigger it since I think we're generally pretty careful about
constructing Joins with Filters, but its removal does increase the
probability that we'll create an invalid Join in the future (possibly
without noticing).
Also change tests so that SimplifyRowNumberOrdering and EliminateNot
trigger now.
One thought I have is that we have lots of these clearly demarcated tests that are intended to test a particular rule - perhaps we should consider introducing a directive which fails a test if a certain set of rules aren't triggered.
Release note: None