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

colexec: fix AND and OR projections in some cases #42129

Merged
merged 1 commit into from
Nov 6, 2019

Conversation

yuzefovich
Copy link
Member

Previously, the original batch length was not respected when the
selection vector is present. This resulted in, for example, query 19 of
TPCH benchmark to return an error. This is now fixed.

I have troubles coming up with a reduced reproduction though.

I also checked that on release-19.2 branch the query is executed
correctly with vectorized, so it must be the switch to flat bytes that
triggers the problem.

Release note: None

@yuzefovich yuzefovich requested review from asubiotto and a team November 2, 2019 03:27
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@asubiotto asubiotto left a comment

Choose a reason for hiding this comment

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

Interesting. Can you repro this with a filter on a non-indexed column before an AND? This probably needs a backport, right?

This seems like it could be a bug that could be easily introduced in any kind of operator. I wonder if there's a way that we could have caught this automatically.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @asubiotto)

Previously, the original batch length was not respected when the
selection vector is present. This resulted in, for example, query 19 of
TPCH benchmark to return an error. This is now fixed.

I have troubles coming up with a reduced reproduction though.

Release note: None
@yuzefovich
Copy link
Member Author

I spent some more time trying to reproduce it, but I didn't succeed. I think in order to trigger this problem you need 1. several batches (i.e. it will not trigger on the first one) and 2. the length of the second batch be shorter than the first one and that that second batch has an interesting selection vector. I feel like it's not worth spending more time trying to come up with a repro.

Yes, it will need to be backported.

Copy link
Contributor

@asubiotto asubiotto 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 1 of 1 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

craig bot pushed a commit that referenced this pull request Nov 6, 2019
37583: storage: Migrate MVCCStats.contains_estimates from bool to int64 r=tbg a=giorgosp

This migration allows ContainsEstimates to be reset after a stats
recomputation (by returning a -ContainsEstimates delta) without
worrying about a race condition. Another command may add 1 to it
flag and it will still be stored as valid.

Resolves #37120

Release note: None

42129: colexec: fix AND and OR projections in some cases r=yuzefovich a=yuzefovich

Previously, the original batch length was not respected when the
selection vector is present. This resulted in, for example, query 19 of
TPCH benchmark to return an error. This is now fixed.

I have troubles coming up with a reduced reproduction though.

I also checked that on release-19.2 branch the query is executed
correctly with vectorized, so it must be the switch to flat bytes that
triggers the problem.

Release note: None

42172: colexec: fix sorted distinct with nulls behavior r=yuzefovich a=yuzefovich

Previously, sorted distinct when the nulls might be present would always
get the value at the index without paying attention whether that value
is actually now. This is incorrect behavior because it is undefined in
some cases (like when getting from flat bytes). Now this is fixed.

Fixes: #42055.

Release note: None

Co-authored-by: George Papadrosou <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
@craig
Copy link
Contributor

craig bot commented Nov 6, 2019

Build succeeded

@craig craig bot merged commit 0a9eb56 into cockroachdb:master Nov 6, 2019
@yuzefovich yuzefovich deleted the fix-and-proj branch November 12, 2019 17:39
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.

3 participants