opt: fix optsteps crash caused by constraint expressions #43405
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)
pkg/sql/opt/xform/testdata/external/foo, line 1 at r1 (raw file):
exec-ddl
Did you mean to add this file to the PR?
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 2 of 2 files at r2, 8 of 8 files at r3.
Reviewable status:
pkg/sql/opt/testutils/opttester/opt_tester.go, line 904 at r1 (raw file):
} next = memo.FormatExpr(os.Root(), ot.Flags.ExprFormat, os.Root().(memo.RelExpr).Memo(), ot.catalog)
why not use the convenience wrapper here and below?
pkg/sql/opt/testutils/opttester/testdata/opt_steps, line 120 at r2 (raw file):
exec-ddl CREATE TABLE customers (
Lots of extra spaces here and below
During debugging, it's sometimes useful to add a statement to print an expression. This is fairly hard and can crash easily because the memo or the catalog isn't set in the formatting context. In particular, it is impossible to print a scalar expression that contains a relational expression because `FormatExpr` only sets the memo when we print relational expressions. This change improves the situation by allowing the caller of `FormatExpr` to pass the `*Memo` and by adding a convenience wrapper method on the `Optimizer`. We also automatically unset the "fully qualify" flag if there is no catalog (which would otherwise crash). Release note: None
The optbuilder creates scalar constraint and computed column expressions and hangs them off the `TableMeta`. If a normalization rule fires for one of these expressions, `optsteps` panics because it can't find a path to the expression in the memo. This change fixes the problem by adding special code in opt_steps which effectively treats these expressions as children of Scan expressions. Release note: None
RaduBerinde
force-pushed
the
optsteps
branch
from
b5de2f0 to
fe0d054
Compare
There was a problem hiding this comment.
Updated the commits to apply the same treatment to the new computed column expressions.
Reviewable status:
pkg/sql/opt/testutils/opttester/opt_tester.go, line 904 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
why not use the convenience wrapper here and below?
Done.
pkg/sql/opt/testutils/opttester/testdata/opt_steps, line 120 at r2 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
Lots of extra spaces here and below
Done.
pkg/sql/opt/xform/testdata/external/foo, line 1 at r1 (raw file):
Previously, andy-kimball (Andy Kimball) wrote…
Did you mean to add this file to the PR?
Heh, no, fixed.
There was a problem hiding this comment.
Reviewed 3 of 11 files at r4, 1 of 2 files at r5, 17 of 17 files at r6.
Reviewable status:
|
Interesting, we do have the colIDs, I'll try to think of something. Maybe show them as a list as the first child, like we would do on a project |
We build check constraint and computed column expressions and attach them to `TableMeta`, to be used later by exploration rules. These are currently invisible. This change adds them under "canonical" scans (the normalized scan expression before any exploration rules). Release note: None
|
Done, I added each computed column as the parent of its expression. |
RaduBerinde
force-pushed
the
optsteps
branch
from
fe0d054 to
6cb068d
Compare
There was a problem hiding this comment.
Nice! This will be really helpful.
Reviewed 14 of 14 files at r7.
Reviewable status:
|
TFTRs! bors r+ |
43405: opt: fix optsteps crash caused by constraint expressions r=RaduBerinde a=RaduBerinde #### opt: make FormatExpr easier to use During debugging, it's sometimes useful to add a statement to print an expression. This is fairly hard and can crash easily because the memo or the catalog isn't set in the formatting context. In particular, it is impossible to print a scalar expression that contains a relational expression because `FormatExpr` only sets the memo when we print relational expressions. This change improves the situation by allowing the caller of `FormatExpr` to pass the `*Memo` and by adding a convenience wrapper method on the `Optimizer`. We also automatically unset the "fully qualify" flag if there is no catalog (which would otherwise crash). Release note: None #### opt: fix optsteps crash caused by TableMeta expressions The optbuilder creates scalar constraint and computed column expressions and hangs them off the `TableMeta`. If a normalization rule fires for one of these expressions, `optsteps` panics because it can't find a path to the expression in the memo. This change fixes the problem by adding special code in opt_steps which effectively treats these expressions as children of Scan expressions. Release note: None #### opt: show TableMeta expressions when formatting expressions We build check constraint and computed column expressions and attach them to `TableMeta`, to be used later by exploration rules. These are currently invisible. This change adds them under "canonical" scans (the normalized scan expression before any exploration rules). Release note: None 43424: reducesql: also attempt to remove columns from PKs r=mjibson a=mjibson Release note: None Co-authored-by: Radu Berinde <[email protected]> Co-authored-by: Matt Jibson <[email protected]>
Build succeededAnd happy new year from bors! 🎉 |
opt: make FormatExpr easier to use
During debugging, it's sometimes useful to add a statement to print an
expression. This is fairly hard and can crash easily because the memo
or the catalog isn't set in the formatting context. In particular, it
is impossible to print a scalar expression that contains a relational
expression because
FormatExpronly sets the memo when we printrelational expressions.
This change improves the situation by allowing the caller of
FormatExprto pass the*Memoand by adding a convenience wrappermethod on the
Optimizer. We also automatically unset the "fullyqualify" flag if there is no catalog (which would otherwise crash).
Release note: None
opt: fix optsteps crash caused by TableMeta expressions
The optbuilder creates scalar constraint and computed column
expressions and hangs them off the
TableMeta. If a normalizationrule fires for one of these expressions,
optstepspanics because itcan't find a path to the expression in the memo.
This change fixes the problem by adding special code in opt_steps
which effectively treats these expressions as children of Scan
expressions.
Release note: None
opt: show TableMeta expressions when formatting expressions
We build check constraint and computed column expressions and attach
them to
TableMeta, to be used later by exploration rules. These arecurrently invisible. This change adds them under "canonical" scans
(the normalized scan expression before any exploration rules).
Release note: None