opt: don't propagate rule props when corresponding rule is disabled #86631
Conversation
|
This change is |
There was a problem hiding this comment.
Thanks for tracking this down!
Reviewed 6 of 7 files at r1, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)
pkg/sql/opt/norm/testdata/rules/prune_cols line 5189 at r1 (raw file):
rowid INT8 NOT VISIBLE NOT NULL DEFAULT unique_rowid(), CONSTRAINT table3_pkey PRIMARY KEY (rowid ASC), UNIQUE INDEX table3_col3_4_col3_11_col3_1_col3_0_col3_3_key (col3_4 DESC, col3_11 ASC, col3_1 DESC, col3_0 ASC, col3_3 DESC) STORING (col3_5, col3_8, col3_9, col3_10, col3_13) WHERE ((((col3_9 = 5e-324:::FLOAT8) OR (col3_1 = (-32768):::INT8)) OR (col3_15 = '':::STRING)) AND (col3_3 > 3.4028234663852886e+38:::FLOAT8)) AND (col3_12 != '""':::STRING)
nit: convert tabs to spaces
pkg/sql/opt/norm/testdata/rules/prune_cols line 5196 at r1 (raw file):
# GroupBy. Then, since PruneWindowInputCols was disabled, # EliminateGroupByProject would remove the Project and the cycle would repeat. norm disable=PruneWindowInputCols
nit: for tests where the opt tree output isn't important for the test, I like to add format=hide-all to make it less verbose. Up to you if you want to do that here.
pkg/sql/opt/xform/optimizer.go line 976 at r1 (raw file):
// caller to set NotifyOnMatchedRule, if desired. func (o *Optimizer) SetDisabledRules(disabledRules RuleSet) { o.disabledRules.UnionWith(disabledRules)
Are you sure modifing o.disabledRules it necessary? It is only used in disableRulesRandom, which calls SetDisabledRules manually.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/norm/testdata/rules/prune_cols line 5189 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: convert tabs to spaces
Done.
pkg/sql/opt/norm/testdata/rules/prune_cols line 5196 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
nit: for tests where the opt tree output isn't important for the test, I like to add
format=hide-allto make it less verbose. Up to you if you want to do that here.
Good idea. Done.
pkg/sql/opt/xform/optimizer.go line 976 at r1 (raw file):
Previously, mgartner (Marcus Gartner) wrote…
Are you sure modifing
o.disabledRulesit necessary? It is only used indisableRulesRandom, which callsSetDisabledRulesmanually.
It shouldn't be necessary, but it seems safest to keep it consistent to avoid confusion if/when this logic gets modified in the future. What do you think?
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status:
pkg/sql/opt/xform/optimizer.go line 976 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
It shouldn't be necessary, but it seems safest to keep it consistent to avoid confusion if/when this logic gets modified in the future. What do you think?
IMO this doesn't avoid confusion, it adds confusion. If I come across this I'd ask myself: "Why was this put here at all? It doesn't seem necessary". The UnionWith adds more confusion because I need to consider cases where o.disabledRules is already non-empty. If I keep calling SetDisabledRules then o.disabledRules continues to grow but o.f.disabledRules is set as the latest set of disabled rules - is that intended or a bug?
I think all this is a sign that the multiple mechanisms for disabling rules (NotifyOnMatchHander) and o.disabledRules are confusing and should be consolidated. I attempted to clean this up last time I ran into it, but didn't quickly come up with a simpler mechanism, so I left it for now. Up to you if you want to leave as-is for now.
|
I've tried to capture the minimal set of changes needed to avoid the rule cycles for the randomized testing, so now the disabled rules logic only applies to the factory, and only when rules are randomly disabled. I also tried unifying the rule-disabling mechanisms here, but I'm not sure if the complexity is worth it. |
There was a problem hiding this comment.
Very nice!
Reviewed 6 of 6 files at r3, all commit messages.
Reviewable status:
This commit decreases the likelihood of rule cycles occuring during optimizer tests with randomly disabled rules. `DerivePruneCols` and `DeriveRejectNullCols` now check whether propagating their corresponding properties would trigger a disabled rule (if it wasn't disabled), and avoid propagating in that case. This is necessary to avoid cases where a `Select` or `Project` gets repeatedly pushed down and eliminated. Addresses cockroachdb#86308 Release note: None Release justification: testing-only change
|
Most recent changes are to notify the factory of disabled rules in the case when they are set as disabled during testing, rather than just randomly. |
|
TFTR! |
|
bors r+ |
|
Build succeeded: |
This commit decreases the likelihood of rule cycles occuring during optimizer
tests with disabled rules.
DerivePruneColsandDeriveRejectNullColsnow check whether propagating their corresponding properties would trigger
a disabled rule (if it wasn't disabled), and avoid propagating in that case.
This is necessary to avoid cases where a
SelectorProjectgets repeatedlypushed down and eliminated.
Addresses #86308
Release note: None
Release justification: testing-only change