andygrove opened a new pull request, #26318:
URL: https://github.com/apache/datafusion/pull/26318
## Which issue does this PR close?
- Closes #26304.
## Rationale for this change
`concat` and `concat_ws` build their output with an offset-based and a view
builder for strings in `strings.rs`, and copies of both for binary in
`binaries.rs`. The copies differ only in the output type, the UTF-8 check on
scalars and `.as_bytes()` on string values, so a fix to one of them, such as to
the view layout in `append_offset`, has to be made twice.
## What changes are included in this PR?
- `ConcatByteArrayBuilder<T>` replaces `ConcatGenericStringBuilder<O>` and
`ConcatGenericBinaryBuilder<O>`, and `ConcatByteViewBuilder<T: ByteViewType>`
replaces `ConcatStringViewBuilder` and `ConcatBinaryViewBuilder`. Arrow's
`ByteArrayType` and `ByteViewType` already give the data type and the array to
build, so `finish` is shared too, with one `SAFETY` comment.
- The one new trait, `ConcatByteArrayType`, adds to `ByteArrayType` the
`IS_UTF8` constant that `ByteViewType` already has. `T::DATA_TYPE.is_string()`
folds to the right constant, but still builds a `DataType` and calls its
out-of-line destructor on every call, which made `concat` on `Binary` about 10%
slower.
- `ColumnarValueRef::write_bytes` replaces the four per-builder matches over
the input column. As before, string outputs take string arrays and UTF-8
scalars, and binary outputs take binary arrays and any scalar, which is what
the unchecked constructors in `finish` rely on. It passes the bytes to a
closure, so each arm appends directly as before. Returning an `Option<&[u8]>`
instead, which sent every arm through one shared append, made `concat` up to
12% slower.
- The six builder names stay as type aliases, so `concat.rs` and
`concat_ws.rs` are unchanged and the binary aliases stay in `binaries.rs`.
- The view builders' `append_offset` uses `MAX_INLINE_LEN` instead of a
literal 12.
The internal error for an input of the wrong type no longer distinguishes
the view builders: it says "string builder" or "binary builder".
## What is the testing strategy for this PR?
- The string builders are covered by the existing `concat` and `concat_ws`
tests, including `string_query.slt.part`, which runs on `Utf8`, `LargeUtf8` and
`Utf8View` columns. The binary builders were only reached by a one-row `Binary`
table, so `string/concat.slt` gets cases that run both functions on `Binary`,
`LargeBinary` and `BinaryView` columns, with a NULL value, a NULL separator, a
non-UTF-8 literal, and a 13-byte `BinaryView` result, one byte over the inline
limit. They pass on `main` too. Raising the builder's inline limit by one makes
the 13-byte row come back with another row's bytes.
- New unit test `concat_builders_check_input_types` checks which inputs each
of the six builders accepts. Coercion keeps SQL from reaching the rejected
ones, but those checks are what keep the unchecked constructors sound. Removing
the UTF-8 check on scalars, or the guard on one binary arm, fails it.
- `cargo test --profile ci --workspace --lib --tests --bins --features
avro,json,backtrace,extended_tests,recursive_protection,parquet_encryption`,
excluding the examples, benchmarks and CLI crates, passes, as does the full
sqllogictest suite.
`concat` and `concat_ws` benchmarks against `main`, the median of five
interleaved runs of each binary on an Apple M3 Max that was busy with other
work:
| Case | `main` | This PR | Change |
| --- | --- | --- | --- |
| `concat`, `Utf8`, 1024 rows | 18.1 µs | 17.8 µs | −1.6% |
| `concat`, `Utf8`, 4096 rows | 71.6 µs | 69.8 µs | −2.4% |
| `concat`, `Utf8`, 8192 rows | 143.3 µs | 139.4 µs | −2.7% |
| `concat`, `Utf8View`, 1024 rows | 27.7 µs | 27.5 µs | −0.6% |
| `concat`, `Utf8View`, 4096 rows | 123.6 µs | 123.2 µs | −0.3% |
| `concat`, `Utf8View`, 8192 rows | 269.1 µs | 262.6 µs | −2.4% |
| `concat`, scalar | 621 ns | 648 ns | +4.3% |
| `concat_ws`, `Utf8`, 1024 rows | 24.5 µs | 23.4 µs | −4.3% |
| `concat_ws`, `Utf8`, 4096 rows | 95.7 µs | 93.4 µs | −2.4% |
| `concat_ws`, `Utf8`, 8192 rows | 191.6 µs | 185.1 µs | −3.4% |
| `concat_ws`, scalar | 646 ns | 622 ns | −3.7% |
| `concat`, `LargeUtf8`, 8192 rows | 144.5 µs | 144.6 µs | +0.0% |
| `concat`, `Binary`, 8192 rows | 126.7 µs | 128.8 µs | +1.7% |
| `concat`, `LargeBinary`, 8192 rows | 124.3 µs | 129.0 µs | +3.7% |
| `concat`, `BinaryView`, 8192 rows | 176.8 µs | 168.1 µs | −4.9% |
| `concat_ws`, `LargeUtf8`, 8192 rows | 192.1 µs | 186.8 µs | −2.8% |
| `concat_ws`, `Binary`, 8192 rows | 174.5 µs | 167.3 µs | −4.2% |
| `concat_ws`, `LargeBinary`, 8192 rows | 171.3 µs | 167.8 µs | −2.0% |
| `concat_ws`, `BinaryView`, 8192 rows | 208.0 µs | 205.3 µs | −1.3% |
The existing benchmarks only cover `Utf8` and `Utf8View`, so the last eight
rows come from a temporary local benchmark, not included here, that runs
`concat(a, ', ', a)` and `concat_ws(', ', a, a)` the same way. The scalar cases
don't use the builders, so they show the noise level of this run.
## Are there any user-facing changes?
No.
--
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]
