Fix UPDATE with multiple identical values#9295
Conversation
djsagain
force-pushed
the
david.stryker/fix-update-duplicates
branch
from
50d47e1 to
ee2c3ea
Compare
There was a problem hiding this comment.
This check is a noop now. We are iterating over columnValueAndRowIdSymbols incrementing symbolCounter in every iteration. It will always be true no matter what
There was a problem hiding this comment.
It feels to me that we should have verify(index >= 0) here. Is that valid case that there exists a symbol in columnValueAndRowIdSymbols which is not part of outputSymbols?
There was a problem hiding this comment.
Good point - - added
verify(index >= 0, "Could not find symbol %s in the outputSymbols %s", symbol, outputSymbols);
losipiuk
left a comment
losipiuk
left a comment
There was a problem hiding this comment.
Well - not relly LGTM. Some comments :)
djsagain
force-pushed
the
david.stryker/fix-update-duplicates
branch
from
ee2c3ea to
584ed00
Compare
|
Thanks for the review, @losipiuk! I acted on your comments, and also fixed one of the added tests whose results depended on |
There was a problem hiding this comment.
These log messages are not particularly useful and will just clutter the test output. Please remove them.
There was a problem hiding this comment.
I removed them and force-pushed.
UPDATE failed when more than one column was updated to the same value. The root cause was confusion in LocalExecutionPlanner.createColumnValueAndRowIdChannels(). This commit fixes that confusion and removes an comment declaring incorrectly that the code depended on order of columns in the page - - instead, the code is looking up the channel by Symbol. This commit adds the test contained in the original bug report, as well as a test for identical update values for multiple columns.
djsagain
force-pushed
the
david.stryker/fix-update-duplicates
branch
from
584ed00 to
4e1cc58
Compare
|
Thanks. Merged |
3 participants
UPDATE failed when more than one column was updated
to the same value. The root cause was confusion in
LocalExecutionPlanner.createColumnValueAndRowIdChannels().
This commit fixes that confusion and removes an comment
declaring incorrectly that the code depended on order of
columns in the page - - instead, the code is looking up
the channel by Symbol.
This commit adds the test contained in the original bug
report, as well as a test for identical update values
for multiple columns.