andygrove commented on code in PR #5215:
URL: https://github.com/apache/datafusion-comet/pull/5215#discussion_r3960773495
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -225,4 +245,122 @@ 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();
+ let ColumnarValue::Array(arr) = expr.evaluate(&batch).unwrap() else {
+ panic!("expected array result");
+ };
+ // All-valid input must not allocate a null buffer (filter fast path).
+ assert!(arr.nulls().is_none());
+ assert_bool_results(ColumnarValue::Array(arr), &[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"]));
+ // Null in dictionary values (keys all valid): is_match emits null,
take carries it.
+ let utf8_values_with_null: ArrayRef =
+ Arc::new(StringArray::from(vec![Some("Rose"), None,
Some("Daisy")]));
+
+ let cases: Vec<(DataType, ArrayRef)> = vec![
+ (
+ DataType::Dictionary(Box::new(DataType::Int32),
Box::new(DataType::Utf8)),
+ Arc::new(DictionaryArray::<Int32Type>::new(
+ Int32Array::from(vec![Some(0), None, Some(1)]),
+ Arc::clone(&utf8_values),
+ )),
+ ),
+ (
+ DataType::Dictionary(Box::new(DataType::Int32),
Box::new(DataType::Utf8View)),
+ Arc::new(DictionaryArray::<Int32Type>::new(
+ Int32Array::from(vec![Some(0), None, Some(1)]),
+ Arc::clone(&utf8_view_values),
+ )),
+ ),
+ (
+ DataType::Dictionary(Box::new(DataType::Int8),
Box::new(DataType::Utf8)),
+ Arc::new(DictionaryArray::<Int8Type>::new(
+ Int8Array::from(vec![Some(0), None, Some(1)]),
+ Arc::clone(&utf8_values),
+ )),
+ ),
Review Comment:
Two more rows here that would each be one line. `Dictionary(UInt64, Utf8)`,
since `as_any_dictionary()` covers the unsigned key types too and both of the
current cases are signed. And a sliced dictionary, since that is the shape that
turns up after a filter or a limit rather than the contiguous one. I confirmed
both panic on `main` and pass on your branch.
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -225,4 +245,122 @@ 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();
+ let ColumnarValue::Array(arr) = expr.evaluate(&batch).unwrap() else {
+ panic!("expected array result");
+ };
+ // All-valid input must not allocate a null buffer (filter fast path).
Review Comment:
The assertion is worth keeping, but the reason in the comment is mine from
the last round and it was wrong. `FilterBuilder::new_with_count` in
arrow-select 59.3.0 dispatches on `filter.null_count()` rather than on
`nulls().is_some()`, so a present-but-all-valid null buffer would not have hit
`prep_null_mask_filter` either way.
What the assertion really pins down is the output shape and the `len / 8`
bytes per batch, and that `BooleanBuilder`'s lazy null buffer keeps behaving
this way across arrow upgrades. Could you reword it to say that instead of
"filter fast path"?
##########
native/spark-expr/src/predicate_funcs/rlike.rs:
##########
@@ -225,4 +245,122 @@ 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"),
+ ])),
+ ),
Review Comment:
All the `Utf8View` values in these tests are `"Rose"` and `"Daisy"`, which
are both 12 bytes or fewer, so they live inline in the view struct. The
representation that actually differs from `Utf8` is the one for strings longer
than 12 bytes, where the view holds a `(len, prefix, buffer_index, offset)`
pointer into a separate data buffer, and that path is never constructed here.
Since `Utf8View` support is the headline of this PR and there is no
end-to-end coverage behind it, could you make one of these values longer than
12 bytes? I checked that a 42-byte value passes on your branch and panics on
`main`, so it is in scope. The `Dictionary(Int32, Utf8View)` case below would
benefit from the same thing.
--
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]
