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 recent problem with hash joiner #53216

Merged
merged 1 commit into from Aug 21, 2020

Conversation

yuzefovich
Copy link
Member

@yuzefovich yuzefovich commented Aug 21, 2020

#53169 has just introduced an optimization in populating of toCheck
slice by having a default hjInitialToCheck slice pre-populated.
However, it could be of insufficient length due to randomization of
coldata.BatchSize() which is now fixed.

Release note: None

@yuzefovich yuzefovich requested review from asubiotto and a team August 21, 2020 17:40
@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.

:lgtm:

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

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

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.

I'm slightly confused though. The error is panic: runtime error: slice bounds out of range [:1588] with capacity 1024. Doesn't that imply that batchSize is larger than it should be?

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

@yuzefovich
Copy link
Member Author

I believe what happens is that init() function is executed before we randomize coldata.BatchSize() value in colexec/main_test.go, so we always create hjInitialToCheck of 1024 length, then we randomize the batch size at which point we might get out of bounds error.

53169 has just introduced an optimization in populating of `toCheck`
slice by having a default `hjInitialToCheck` slice pre-populated.
However, it could be of insufficient length due to randomization of
`coldata.BatchSize()` which is now fixed.

Release note: None
@craig
Copy link
Contributor

craig bot commented Aug 21, 2020

Canceled.

@yuzefovich
Copy link
Member Author

I've simply adjusted the comment.

bors r+

@yuzefovich
Copy link
Member Author

bors p=9999

@yuzefovich
Copy link
Member Author

bors r+ p=9999

@craig
Copy link
Contributor

craig bot commented Aug 21, 2020

Already running a review

@otan
Copy link
Contributor

otan commented Aug 21, 2020

i think your series of commands broke bors :P

@craig
Copy link
Contributor

craig bot commented Aug 21, 2020

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Aug 21, 2020

Build succeeded:

@craig craig bot merged commit cab0b31 into cockroachdb:master Aug 21, 2020
@yuzefovich yuzefovich deleted the fix-hj branch August 21, 2020 21:23
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.

None yet

4 participants