type coercion: support is/is_not_bool/like/unknown expr
#3510
Conversation
github-actions
bot
added
logical-expr
|
We will add the I also has the question about that #3396 (comment) If we can rewrite these logical expr to binary expr, we can reuse type coercion logic for the binary expr and don't need to add much more match branch for each expr which need type coercion. |
Codecov Report
@@ Coverage Diff @@
## master #3510 +/- ##
==========================================
+ Coverage 85.75% 85.91% +0.16%
==========================================
Files 299 301 +2
Lines 55311 56218 +907
==========================================
+ Hits 47431 48299 +868
- Misses 7880 7919 +39
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Co-authored-by: Daniël Heres <[email protected]>
liukun4515
force-pushed
the
support_bool_type_coercion_#3509
branch
from
52de4d2 to
67696c2
Compare
liukun4515
changed the title
bool exprbool/like/unknown expr
|
@alamb @andygrove @Dandandan PTAL |
|
Thanks @liukun4515 I will review this today or tomorrow |
| pattern, | ||
| escape_char, | ||
| }; | ||
| expr.rewrite(&mut self.const_evaluator) |
There was a problem hiding this comment.
🤔 maybe in the long term figuring out how to avoid having to remember to call the const evaluator each struct would be good -- as it too does a expr tree walk
So this algorithm is at least O(n^2) in the number of items in the expr tree.
I don't think it is important to do in this PR, but I think it is worth keeping in mind as we proceed or if we see the planner being a bit slow
There was a problem hiding this comment.
maybe in the long term figuring out how to avoid having to remember to call the const evaluator each struct would be good -- as it too does a expr tree walk
agree with that
There was a problem hiding this comment.
I think the SQL level with analyse/expr/optimizer should be refactor.
We are missing some formal logic like spark SQL framework or other SQL framework.
This makes it very difficult for us to do some features or optimization.
There was a problem hiding this comment.
I agree the current framework for expr rewriting / analysis is reaching the limits of its existing design. Refactoring the code so it is easier to work with sounds like a good idea to me.
I am open to suggestions -- I think the challenge in any such refactors is not only coming up with a good new design but also figuring out how to incrementally get the existing code into that new structure
| let left_type = expr.get_type(&self.schema)?; | ||
| let right_type = DataType::Boolean; | ||
| let coerced_type = | ||
| coerce_types(&left_type, &Operator::IsNotDistinctFrom, &right_type)?; |
There was a problem hiding this comment.
Is there a reason this isn't Operator::IsUnknown?
| let left_type = expr.get_type(&self.schema)?; | ||
| let right_type = DataType::Boolean; | ||
| let coerced_type = | ||
| coerce_types(&left_type, &Operator::IsDistinctFrom, &right_type)?; |
There was a problem hiding this comment.
Likewise here, it seem strange that this is not Operator::IsNotUnknown
waiting for your review. |
Apologies. I have not had time. I see that @alamb has approved this so please don't let me hold this up. |
Co-authored-by: Andrew Lamb <[email protected]>
Thanks for your reply. You can comment it after it merged. |
|
Benchmark runs are scheduled for baseline = aad82fb and contender = 11abfb9. 11abfb9 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3509
block the pr #3396
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?