andygrove commented on code in PR #6824:
URL: https://github.com/apache/datafusion-comet/pull/6824#discussion_r4233686731
##########
native/shuffle/src/spark_unsafe/row.rs:
##########
@@ -668,8 +668,12 @@ fn append_list_column_batch(
DataType::Date32 => {
process_primitive_lists!(Date32Builder, append_dates_to_builder);
}
- DataType::Timestamp(TimeUnit::Microsecond, _) => {
- process_primitive_lists!(TimestampMicrosecondBuilder,
append_timestamps_to_builder);
+ DataType::Timestamp(TimeUnit::Microsecond, tz) => {
+ process_primitive_lists!(
+ TimestampMicrosecondBuilder,
+ append_timestamps_to_builder,
Review Comment:
Added `timestamp_list_column_keeps_its_timezone` in `row.rs` (29c8d7935a).
It lays out `UnsafeRow`s with one `Timestamp(Microsecond, Some("UTC"))` list
column and runs them through `append_columns` with the builder `make_builders`
returns, so it goes through `append_list_column_batch`. The rows hold a
70-element array with a null, a null list, a two-element array with a null and
a 70-element array without nulls, so all three append paths write into one
values builder. With `&None` in place of `tz`, it fails on `append_array`'s
data type assertion.
I also added `columnar shuffle on array [timestamp]` to
`CometColumnarShuffleSuite`. It shuffles rows that each hold a 70-element
`array<timestamp>` with a null, at 10 and 201 partitions. It passes on Spark
4.1 and 3.5. Against a `libcomet` built with `&None`, it fails in both
`CometShuffleSuite` and `DisableAQECometShuffleSuite` with
`CometNativeException: assertion left == right failed: array data type
mismatch`.
##########
native/shuffle/src/spark_unsafe/list.rs:
##########
@@ -39,55 +44,90 @@ use datafusion_comet_jni_bridge::errors::CometError;
/// time with the copy, and arrays of 50 about a fifth.
const MIN_BULK_APPEND_ELEMENTS: usize = 8;
+/// Element count from which an aligned nullable array that holds a null is
appended with one copy
+/// of its values and a validity buffer built from its null bitset, instead of
element by element.
+/// Building the two buffers takes four allocations, which cost as much as
appending 40 to 55
+/// elements one by one, so shorter arrays stay on the loop: with the copy,
arrays of 16 elements
+/// took 2.4 times as long and arrays of 32 elements 1.3 times. Arrays of 64
elements take 65% of
+/// the time with the copy for 4-byte elements and 85% for 8-byte ones, and
arrays of 1024 a ninth
+/// and a third.
+const MIN_BULK_NULLABLE_APPEND_ELEMENTS: usize = 64;
+
/// Generates bulk append methods for primitive types in SparkUnsafeArray.
///
/// # Safety invariants for all generated methods:
/// - `element_offset` points to contiguous element data of length
`num_elements`
/// - `null_bitset_ptr()` returns a pointer to `ceil(num_elements/64)` i64
words
/// - These invariants are guaranteed by the SparkUnsafeArray layout from the
JVM
+///
+/// A timestamp appender also takes the column's timezone, because
`append_array` requires the
Review Comment:
Done in 29c8d7935a. The macro doc now says `timezone` must be the builder's
timezone, and that a mismatch panics in `append_array` for an aligned array of
`MIN_BULK_NULLABLE_APPEND_ELEMENTS` or more elements that holds a null and goes
unnoticed for every other array. The `has_null()` comment now says a stray
padding bit can send the array to the per-element loop or to the validity
buffer, and that both ignore the bits past the last element.
--
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]
