feat: Support NOT and column expressions in eval_sql_where #653
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
- Coverage 84.02% 84.01% -0.01%
==========================================
Files 75 75
Lines 17075 17097 +22
Branches 17075 17097 +22
==========================================
+ Hits 14347 14364 +17
+ Misses 2044 2043 -1
- Partials 684 690 +6 ☔ View full report in Codecov by Sentry. |
kernel/src/predicates/mod.rs
Outdated
| ]; | ||
| self.finish_eval_variadic(VariadicOperator::And, exprs, false) | ||
| } | ||
| _ => self.eval_expr(filter, false), | ||
| // NOTE: It's unsafe to process other expressions like DISTINCT, IS NULL, and literals |
There was a problem hiding this comment.
this warning is a good addition 👌
kernel/src/predicates/mod.rs
Outdated
| ]; | ||
| self.finish_eval_variadic(VariadicOperator::And, exprs, false) | ||
| } | ||
| _ => self.eval_expr(filter, false), | ||
| // NOTE: It's unsafe to process other expressions like DISTINCT, IS NULL, and literals | ||
| // with null-safe semantics, so just process them normally. |
There was a problem hiding this comment.
nit: can we add some of the context you sent me on slack @scovich 😆
e.g. the example of FALSE dominating AND and giving unconditional (incorrect) skipping?
There was a problem hiding this comment.
The case I worried about was:
AND(NULL, ...)
= AND(NULL IS NOT NULL AND NULL, ...)
= AND(FALSE AND NULL, ...)
= AND(FALSE, ...)
= FALSE
However, from SQL perspective, that behavior is actually correct -- the (unmodified) filter can never evaluate to TRUE, so such rows will always be dropped. SQL semantics just changes the NULL to a FALSE to allow data skipping.
Based on this, I updated the PR to cover NULL literals after all.
|
I'm getting 404 on codecov links (???) but from previous experience the low coverage is due to unit tests that don't fail and therefore don't exercise their failure messages. |
What changes are proposed in this pull request?
The first attempt at generalizing SQL was incomplete, supporting only AND/OR and binary comparisons. We can also support NULL literal expressions, boolean column expressions and NOT, so add them.
How was this change tested?
Adjusted existing unit tests for the new support (they previously expected it to not work)