Skip to content
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: Inline constant values #33417

Merged
merged 1 commit into from
Jan 3, 2019
Merged

opt: Inline constant values #33417

merged 1 commit into from
Jan 3, 2019

Conversation

andy-kimball
Copy link
Contributor

Inline constants in expressions like:

SELECT x, x+1 FROM (VALUES (1)) AS t(x) ;

with the new inlining rules, this becomes:

VALUES (1, 2)

The new inlining rules are useful for mutation expressions (e.g. UPDATE),
which can nest multiple Project and Values expressions that often use
constant values. For example:

CREATE TABLE ab (a INT PRIMARY KEY, b INT AS (a + 1) STORED);
UPDATE ab SET a=1

This now gets mapped by the optimizer to this internal equivalent:

UPDATE ab SET a=1, b=2

Release note: None

@andy-kimball andy-kimball requested a review from a team as a code owner December 31, 2018 19:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andy-kimball andy-kimball force-pushed the inconst branch 2 times, most recently from 1124b3d to 4e55f39 Compare January 1, 2019 17:36
Copy link
Contributor

@justinj justinj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 9 of 9 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/sql/opt/norm/inline.go, line 25 at r1 (raw file):

// constant value expressions: ConstOp, TrueOp, FalseOp, or NullOp. Constant
// value expressions can often be inlined into referencing expressions. Only
// Project and Values operators synthesize constant value expressions.

What about things like Select operators that have an constant equality condition in their filters? Not sure if that check is also valuable. I guess more generally this is sort of like an FD constant column, except with the restriction that we know the constant value statically. Probably not worth getting super exhaustive on these except in the common cases.


pkg/sql/opt/norm/rules/inline.opt, line 31 at r1 (raw file):

=>
(Project
	$input

nit: you've got some tabs here instead of spaces (and also below)

Inline constants in expressions like:

  SELECT x, x+1 FROM (VALUES (1)) AS t(x) ;

with the new inlining rules, this becomes:

  VALUES (1, 2)

The new inlining rules are useful for mutation expressions (e.g. UPDATE),
which can nest multiple Project and Values expressions that often use
constant values. For example:

  CREATE TABLE ab (a INT PRIMARY KEY, b INT AS (a + 1) STORED);
  UPDATE ab SET a=1

This now gets mapped by the optimizer to this internal equivalent:

  UPDATE ab SET a=1, b=2

Release note: None
Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 9 of 9 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


pkg/sql/opt/norm/testdata/rules/inline, line 154 at r1 (raw file):

 │    ├── scan xy
 │    └── filters
 │         └── 1 > 0 [type=bool]

why is this not getting folded to true?

Copy link
Contributor Author

@andy-kimball andy-kimball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


pkg/sql/opt/norm/inline.go, line 25 at r1 (raw file):

Previously, justinj (Justin Jaffray) wrote…

What about things like Select operators that have an constant equality condition in their filters? Not sure if that check is also valuable. I guess more generally this is sort of like an FD constant column, except with the restriction that we know the constant value statically. Probably not worth getting super exhaustive on these except in the common cases.

I think it's potentially interesting to look for constant equality conditions. I'd do it if/when we find cases where it's important. The cases I'm adding here are useful for mutation cases, since we often "stack" multiple Project/Values operators, and I was noticing we weren't able to reduce them effectively.


pkg/sql/opt/norm/testdata/rules/inline, line 154 at r1 (raw file):

Previously, rytaft wrote…

why is this not getting folded to true?

It appears that we're not normalizing GT to LT (see tree.foldComparisonExpr) when doing folding. I can file another issue on this, as it's orthogonal to this PR.

Copy link
Collaborator

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale)


pkg/sql/opt/norm/testdata/rules/inline, line 154 at r1 (raw file):

Previously, andy-kimball (Andy Kimball) wrote…

It appears that we're not normalizing GT to LT (see tree.foldComparisonExpr) when doing folding. I can file another issue on this, as it's orthogonal to this PR.

Sounds good.

@andy-kimball
Copy link
Contributor Author

bors r+

craig bot pushed a commit that referenced this pull request Jan 3, 2019
33312: sql/opt: avoid swallowing TransactionAbortedErrors r=benesch a=benesch

An optimizer code path could, in rare cases, fail to propagate a
transaction aborted error. This proved disastrous as, due to a footgun
in our transaction API (#22615), swallowing a transaction aborted error
results in proceeding with a brand new transaction that has no knowledge
of the earlier operations performed on the original transaction.

This presented as a rare and confusing bug in splits, as the split
transaction uses an internal executor. The internal executor would
occasionally silently return a new transaction that only had half of the
necessary operations performed on it, and committing that partial
transaction would result in a "range does not match splits" error.

Fixes #32784.

Release note: None

/cc @tbg

33417: opt: Inline constant values r=andy-kimball a=andy-kimball

Inline constants in expressions like:

  SELECT x, x+1 FROM (VALUES (1)) AS t(x) ;

with the new inlining rules, this becomes:

  VALUES (1, 2)

The new inlining rules are useful for mutation expressions (e.g. UPDATE),
which can nest multiple Project and Values expressions that often use
constant values. For example:

  CREATE TABLE ab (a INT PRIMARY KEY, b INT AS (a + 1) STORED);
  UPDATE ab SET a=1

This now gets mapped by the optimizer to this internal equivalent:

  UPDATE ab SET a=1, b=2

Release note: None

33421: opt: Tighten up InlineProjectInProject rule r=andy-kimball a=andy-kimball

Allow inlining nested Project in case where there are duplicate refs
to same inner passthrough column. Previously, this case prevented
inlining. However, only duplicate references to inner *synthesized*
columns need to be detected.

Release note: None

Co-authored-by: Nikhil Benesch <[email protected]>
Co-authored-by: Andrew Kimball <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jan 3, 2019

Build succeeded

@craig craig bot merged commit c920fe0 into cockroachdb:master Jan 3, 2019
@andy-kimball andy-kimball deleted the inconst branch January 3, 2019 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants