sam-1112 commented on code in PR #5215:
URL: https://github.com/apache/datafusion-comet/pull/5215#discussion_r3706209733
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -111,29 +135,19 @@ impl PhysicalExpr for RLike {
fn evaluate(&self, batch: &RecordBatch) -> Result<ColumnarValue> {
match self.child.evaluate(batch)? {
- ColumnarValue::Array(array) if
array.as_any().is::<DictionaryArray<Int32Type>>() => {
- let dict_array = array
- .as_any()
- .downcast_ref::<DictionaryArray<Int32Type>>()
- .expect("dict array");
- let dict_values = dict_array
- .values()
- .as_any()
- .downcast_ref::<StringArray>()
- .expect("strings");
+ ColumnarValue::Array(array)
+ if matches!(array.data_type(), DataType::Dictionary(_, _)) =>
+ {
+ let dict_array = as_dictionary_array::<Int32Type>(&array)?;
Review Comment:
Good catch — thanks for checking the non-Int32 key case.
You're right: widening the guard to `Dictionary(_, _)` while still casting
with `as_dictionary_array::<Int32Type>` only turned the old panic into an
`Internal` error for other key types. I've switched to
`array.as_any_dictionary()` and `take(&new_values, dict_array.keys(), None)` as
you suggested, and added a `Dictionary(Int8, Utf8)` test that expects `[true,
null, false]`.
Glad to hear `Dictionary(Int32, Utf8View)` / `LargeUtf8` already looked good
— those stay covered as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
