grorge123 commented on code in PR #5526:
URL: https://github.com/apache/datafusion-comet/pull/5526#discussion_r3930889531
##########
native/shuffle/src/spark_unsafe/row.rs:
##########
@@ -1020,6 +1027,13 @@ fn append_struct_fields_field_major(
}
}
}
+ // A Null field carries no data: every row is null, whether or not
the struct is.
+ DataType::Null => {
+ let field_builder = get_field_builder!(struct_builder,
NullBuilder, field_idx);
+ for _ in row_start..row_end {
Review Comment:
Fixed: reproduced first with the sort-based writer
(`spark.shuffle.sort.bypassMergeThreshold=0`,
`spark.comet.shuffle.jvm.batchSize=2`, jvm shuffle mode,
`convertFromSparkPlan.enabled=false`, AQE off, 16 rows into 2 partitions):
`named_struct('v', id, 'n', NULL)` panics in `StructBuilder::finish` with `(2
!= 4)` on the second batch, as you describe, and so do a nested struct and
`element_at(transform(array(id), x -> named_struct('v', x, 'n', NULL)), 1)`.
The same reuse defect hits every other Null-bearing shape: a top-level `NULL`
column fails the batch's row-count check, `map(id, NULL)` fails "keys and
values have unequal length", and `array<null>` comes back with the wrong row
count. Base control at `ef62b463` under the same settings: the top-level `NULL`
column and `array(named_struct('v', id, 'n', NULL))` fail there too (row-count
check and the same `(2 != 4)` panic through the row-major arm), the struct
shapes hit the field-major `unreachable` instead, and `map(id, NULL)`, `a
rray<null>` and the `element_at` shape stayed in Spark on the base, so those
three are admitted by this PR. `NullBuilder::finish` keeps its length in arrow
58.4.0 (a `NullArray` owns no buffers to hand over), so
`process_sorted_row_partition` now recreates every builder whose type holds a
`Null` anywhere (`contains_null_type`) after each batch; the other builders
reset on finish and are kept. The bypass writer sends at most one batch per
native call, which is why the single-batch controls and the sweep's
`REPARTITION(3)` never saw it.
Tests: `null_type_builders_start_every_batch_empty` in row.rs drives two
batches through the production `make_builders` / `builder_to_array` / recreate
path for a Null field, a nested one and a top-level Null column;
`CometColumnarShuffleSuite` "columnar shuffle spanning several native writer
batches with NullType columns" runs the seven shapes above through
`REPARTITION(300)` with `jvm.batchSize=2` and the spill threshold lifted, under
both AQE settings, comparing with Spark.
--
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]
