fix: Support on all-literal RLIKE expression#3647
Conversation
coderfender
left a comment
coderfender
left a comment
There was a problem hiding this comment.
Thank you for adding additional support. Left couple of comments
| let is_match = match scalar { | ||
| ScalarValue::Utf8(Some(s)) => self.pattern.is_match(s.as_str()), | ||
| ScalarValue::LargeUtf8(Some(s)) => self.pattern.is_match(s.as_str()), | ||
| ScalarValue::Utf8View(Some(s)) => self.pattern.is_match(s.as_str()), |
There was a problem hiding this comment.
@0lai0 would we ever get a UTF8View input from the spark side?
There was a problem hiding this comment.
Yes, On the current Spark -> Comet proto -> native planner path, string literals are deserialized as ScalarValue::Utf8. So I think keeping Utf8 handling here is necessary.
Let me know if I missed anything.
| ScalarValue::LargeUtf8(Some(s)) => self.pattern.is_match(s.as_str()), | ||
| ScalarValue::Utf8View(Some(s)) => self.pattern.is_match(s.as_str()), | ||
| _ => { | ||
| return internal_err!( |
| #[cfg(test)] | ||
| use datafusion::physical_expr::expressions::Literal; |
There was a problem hiding this comment.
The import should just be moved inside mod test which is already behind the #[cfg(test)] guard
|
Thanks @andygrove for the review. PR updated. |
| #[test] | ||
| fn test_rlike_scalar_utf8_literal() { | ||
| let expr = RLike::try_new( | ||
| Arc::new(Literal::new(ScalarValue::Utf8(Some("Rose".to_string())))), |
There was a problem hiding this comment.
This tests the happy path (Utf8(Some)).
It would be good to also add a test case for Utf8(None).
You could also extend it to cover the LargeUtf8 and Utf8View paths too:
for scalar in &[ScalarValue::Utf8(Some("Utf8".to_string())), ScalarValue::LargeUtf8(Some("LargeUtf8".to_string())), ScalarValue::Utf8View(Some("Utf8View".to_string()))] { ... }and some non-Utf8 type, e.g. ScalarValue::Boolean to trigger the error.
| let is_match = match scalar { | ||
| ScalarValue::Utf8(Some(s)) | ||
| | ScalarValue::LargeUtf8(Some(s)) | ||
| | ScalarValue::Utf8View(Some(s)) => self.pattern.is_match(s.as_str()), |
There was a problem hiding this comment.
| | ScalarValue::Utf8View(Some(s)) => self.pattern.is_match(s.as_str()), | |
| | ScalarValue::Utf8View(Some(s)) => self.pattern.is_match(&s), |
4 participants
Which issue does this PR close?
Closes #3343
Rationale for this change
When
ConstantFoldingis disabled, all-literal RLIKE expressions crash the native engine with a misleading error. The engine should handle scalar literals gracefully.What changes are included in this PR?
How are these changes tested?
ConstantFoldingdisabledrlike_enabled.sqltest now passes./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite rlike_enabled" -Dtest=none