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: Fix panic when hoisting expr with correlated subquery #32443

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
4 participants
@andy-kimball
Copy link
Contributor

andy-kimball commented Nov 17, 2018

Fixes #32270.

The panic occurs when hoisting an expression that has both a correlated
and an uncorrelated subquery. The current code panics when calling
Reconstruct on the relational input to the uncorrelated subquery. The
fix is to test for relational inputs and skip over them. They are not
correlated, and so no hoisting needs to be done within their subtree.

Release note (sql change): Fix panic when expression contains both a
correlated and uncorrelated subquery.

@andy-kimball andy-kimball requested review from justinj , rytaft and RaduBerinde Nov 17, 2018

@andy-kimball andy-kimball requested a review from cockroachdb/sql-opt-prs as a code owner Nov 17, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

cockroach-teamcity commented Nov 17, 2018

This change is Reviewable

@justinj
Copy link
Member

justinj left a comment

LGTM

// It is not necessary to descend into relational children, because only
// subquery scalar operators have a relational child, and they are handled
// above, either because they're not correlated, or because they've already
// been hoisted.

This comment has been minimized.

@justinj

justinj Nov 17, 2018

Member

I think the commit message explains this a little better than this comment. The comment implies that we skip over both correlated and uncorrelated relational expressions, but I think here the deal is that we (still) won't encounter correlated relational expressions, and we want to skip over the uncorrelated ones, right?

@rytaft

rytaft approved these changes Nov 19, 2018

Copy link
Contributor

rytaft left a comment

[nit] I think you should put the issue number in the body of the commit/PR message rather than the header so that the GitHub workflow will work correctly. If you add a line to the body of the PR message that says "Fixes #32270", the craigbot will update the issue with a reference to this PR, and close the issue when this PR merges.

Also, I think this probably deserves a release note.

Otherwise, :lgtm:, once you address Justin's comment

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

@andy-kimball andy-kimball force-pushed the andy-kimball:decorrelate branch from f61832c to 282c2ef Nov 22, 2018

@andy-kimball
Copy link
Contributor

andy-kimball left a comment

This isn't a problem in 2.1, so I don't think we use a release note in that case, do we? This bug was only introduced by my expression rewrite, so it's an intra-release bug.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


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

Previously, justinj (Justin Jaffray) wrote…

I think the commit message explains this a little better than this comment. The comment implies that we skip over both correlated and uncorrelated relational expressions, but I think here the deal is that we (still) won't encounter correlated relational expressions, and we want to skip over the uncorrelated ones, right?

Rewrote this.

@rytaft
Copy link
Contributor

rytaft left a comment

But was the bug in the first 2.2 alpha release? If so, I would think the next alpha release should still have the release note. But it's definitely less important if the bug is not in 2.1...

(Also - I saw that you updated the commit message, but I think you may also need to update the PR message for the issue to be closed automatically)

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

@andy-kimball andy-kimball changed the title opt: Issue 32270: panic when hoisting expr with correlated subquery opt: Fix panic when hoisting expr with correlated subquery Nov 28, 2018

opt: Fix panic when hoisting expr with correlated subquery
Fixes #32270.

The panic occurs when hoisting an expression that has both a correlated
and an uncorrelated subquery. The current code panics when calling
Reconstruct on the relational input to the uncorrelated subquery. The
fix is to test for relational inputs and skip over them. They are not
correlated, and so no hoisting needs to be done within their subtree.

Release note (sql change): Fix panic when expression contains both a
correlated and uncorrelated subquery.

@andy-kimball andy-kimball force-pushed the andy-kimball:decorrelate branch from 282c2ef to f429b55 Nov 28, 2018

@andy-kimball

This comment has been minimized.

Copy link
Contributor

andy-kimball commented Nov 29, 2018

bors r+

craig bot pushed a commit that referenced this pull request Nov 29, 2018

Merge #32443
32443: opt: Fix panic when hoisting expr with correlated subquery r=andy-kimball a=andy-kimball

Fixes #32270.

The panic occurs when hoisting an expression that has both a correlated
and an uncorrelated subquery. The current code panics when calling
Reconstruct on the relational input to the uncorrelated subquery. The
fix is to test for relational inputs and skip over them. They are not
correlated, and so no hoisting needs to be done within their subtree.

Release note (sql change): Fix panic when expression contains both a
correlated and uncorrelated subquery.

Co-authored-by: Andrew Kimball <andyk@cockroachlabs.com>
@craig

This comment has been minimized.

Copy link

craig bot commented Nov 29, 2018

Build succeeded

@craig craig bot merged commit f429b55 into cockroachdb:master Nov 29, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@andy-kimball andy-kimball deleted the andy-kimball:decorrelate branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment