Rework row matching #8979
Rework row matching #8979
Conversation
There was a problem hiding this comment.
Hey @lnkuiper! Thanks for the PR and for picking this up to properly rework it! 🎉 Looks neat now, and seeing a lot of const there is very nice.
My comments are minor notes on readability. I tried to follow the logic as best as possible, and it looks good to me!
|
Thanks for the review! I think this is good to go. Somehow I'm getting a coverage failure in the csv reader, but that shouldn't be because of my changes, and is probably a spurious failure |
| expr->type <= ExpressionType::COMPARE_GREATERTHANOREQUALTO) || | ||
| expr->type == ExpressionType::COMPARE_DISTINCT_FROM || | ||
| expr->type == ExpressionType::COMPARE_NOT_DISTINCT_FROM) { | ||
| } else if (expr->type == ExpressionType::COMPARE_EQUAL || expr->type == ExpressionType::COMPARE_NOTEQUAL || |
There was a problem hiding this comment.
Unless I'm mistaken this doesn't appear to functionally change anything, it just lists out the enums instead of using a range comparison?
There was a problem hiding this comment.
Yes, @taniabogatsch and I changed this because we found it more readable this way
There was a problem hiding this comment.
It was a leftover from the initial debugging, but we decided to keep it for readability, yes.
|
Thanks! |
Fixes #8328, which triggered this PR.
I've reworked row matching. I initially wanted to do a small patch instead of a rework, but @taniabogatsch and I could not figure out how the old code worked in the first place, as it seemed to handle cases like
COMPARE_EQUALandCOMPARE_NOT_DISTINCT_FROMthe same even though they are not. This is probably why the bug appeared in the first case.Instead of a static function with multiple switches every time it is called, we now initialize a
RowMatcherobject that does the switches exactly once, obtaining the correct function pointers. So, other than correctly matching rows, this PR should also give a slight performance boost.