-
Notifications
You must be signed in to change notification settings - Fork 555
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
fix(expr): fix a critical bug in case expression #13890
Conversation
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
Signed-off-by: Runji Wang <wangrunji0408@163.com>
let calc_then_vis = when.eval(&input).await?.as_bool().to_bitmap(); | ||
let input_vis = input.visibility().clone(); | ||
// note that evaluated result from when clause may contain bits that are not visible, | ||
// so we need to mask it with input visibility. | ||
let calc_then_vis = when.eval(&input).await?.as_bool().to_bitmap() & &input_vis; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thx for the quick fix!
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13890 +/- ##
==========================================
- Coverage 68.33% 68.27% -0.06%
==========================================
Files 1528 1528
Lines 263343 263322 -21
==========================================
- Hits 179958 179793 -165
- Misses 83385 83529 +144
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Runji Wang <wangrunji0408@163.com>
I confirmed that this bug was introduced during v0.1.14 to v0.1.15, by #6581. |
Is this considered a breaking change? 🥵 risingwavelabs/rfcs#24 (comment) @BugenZhao |
Strictly speaking, yes. 😕 We need to introduce a new function type with the fixed implementation, while keeping the buggy one as "backend only". But it's also okay for me to simply assume that no one persisted the result prior to this PR. 🤣 |
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
Sometimes the CASE expression fails to short circuit, causing the last matching value to be returned instead of the first one.
The reason is that some expressions ignore the visibility and return non-null values for invisible rows. Examples include literal expression and simd-optimized expressions such as
=
. The CASE expression doesn't mask invisible rows in the condition result, so later values may override earlier ones.This bug seems to have been there from
day 1v0.1.15 (Jan 2023). It sounds very horrible. 🤯Checklist
./risedev check
(or alias,./risedev c
)Documentation