andygrove commented on code in PR #5215: URL: https://github.com/apache/datafusion-comet/pull/5215#discussion_r3708606278
########## native/spark-expr/src/predicate_funcs/rlike.rs: ########## @@ -37,6 +36,7 @@ use std::sync::Arc; /// regular expression engine, which are documented at: /// /// https://docs.oracle.com/javase/8/docs/api/java/util/regex/Pattern.html +/// Review Comment: Looks like removing the perf paragraph left a bare `///` behind just above `#[derive(Debug)]`. Could you drop that line? It is the only noise left in an otherwise tidy diff. ########## native/spark-expr/src/predicate_funcs/rlike.rs: ########## @@ -225,4 +246,107 @@ mod tests { let result = expr.evaluate(&RecordBatch::new_empty(Arc::new(Schema::empty()))); assert!(result.is_err()); } + + #[test] + fn test_rlike_string_array_layouts() { + let pattern = "R[a-z]+"; + let cases: Vec<(DataType, ArrayRef)> = vec![ + ( + DataType::Utf8, + Arc::new(StringArray::from(vec![Some("Rose"), None, Some("Daisy")])), + ), + ( + DataType::LargeUtf8, + Arc::new(LargeStringArray::from(vec![ + Some("Rose"), + None, + Some("Daisy"), + ])), + ), + ( + DataType::Utf8View, + Arc::new(StringViewArray::from(vec![ + Some("Rose"), + None, + Some("Daisy"), + ])), + ), + ]; + + for (data_type, array) in cases { + let schema = Arc::new(Schema::new(vec![Field::new("s", data_type, true)])); + let batch = RecordBatch::try_new(Arc::clone(&schema), vec![array]).unwrap(); + let expr = RLike::try_new(Arc::new(Column::new("s", 0)), pattern).unwrap(); + assert_bool_results( + expr.evaluate(&batch).unwrap(), + &[Some(true), None, Some(false)], + ); + } + } + + #[test] + fn test_rlike_string_array_no_nulls() { + let schema = Arc::new(Schema::new(vec![Field::new("s", DataType::Utf8, false)])); + let batch = RecordBatch::try_new( + Arc::clone(&schema), + vec![Arc::new(StringArray::from(vec!["Rose", "Daisy"]))], + ) + .unwrap(); + + let expr = RLike::try_new(Arc::new(Column::new("s", 0)), "R[a-z]+").unwrap(); + assert_bool_results(expr.evaluate(&batch).unwrap(), &[Some(true), Some(false)]); + } + + #[test] + fn test_rlike_dictionary_arrays() { + let pattern = "R[a-z]+"; + let expected = [Some(true), None, Some(false)]; + + let utf8_values: ArrayRef = Arc::new(StringArray::from(vec!["Rose", "Daisy"])); + let utf8_view_values: ArrayRef = Arc::new(StringViewArray::from(vec!["Rose", "Daisy"])); + + let cases: Vec<(DataType, ArrayRef)> = vec![ Review Comment: All three dictionary cases put the null in the keys. A null in the dictionary *values* takes a different route to the same place: `is_match` emits it and `take` carries it through, rather than `take` producing it from a null key. I tried it on your branch and it gives the right answer, so this is about pinning the behavior rather than a bug. Since the dictionary arm is the bulk of this diff, would you mind adding a case with values like `[Some("Rose"), None, Some("Daisy")]` and all-valid keys? -- 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]
