andygrove commented on code in PR #5903:
URL: https://github.com/apache/datafusion-comet/pull/5903#discussion_r4238262443
##########
spark/src/test/scala/org/apache/comet/exec/CometAggregateSuite.scala:
##########
@@ -188,6 +188,46 @@ class CometAggregateSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
classOf[LocalTableScanExec])
}
+ test("collect_list and collect_set over non-nullable nested fields survive
spilling") {
+ withTempPath { path =>
+ // Keep every group in one input and final partition so aggregation
outgrows its memory pool.
+ spark
+ .createDataFrame((0 until 32768).map { i =>
+ (i % 4096, i % 8192, s"${i % 8192}-" + "x" * 256)
+ })
+ .coalesce(1)
+ .write
+ .parquet(path.getAbsolutePath)
+ withParquetTable(path.getAbsolutePath, "tbl") {
+ withSQLConf(
+ SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false",
+ SQLConf.SHUFFLE_PARTITIONS.key -> "1",
+ CometConf.COMET_BATCH_SIZE.key -> "1024",
+ CometConf.COMET_OFFHEAP_MEMORY_POOL_FRACTION.key -> "0.002",
+ CometConf.COMET_RESPECT_DATAFUSION_CONFIGS.key -> "true",
+ "spark.comet.datafusion.execution.sort_spill_reservation_bytes" ->
"65536") {
+ // The literal and coalesce create required fields after the Parquet
scan, which
+ // otherwise widens field nullability. Sorting removes collect order
differences.
+ val struct = "named_struct('flag', true, 'id', coalesce(_2, 0),
'value', _3)"
+ Seq(struct, s"array($struct)").foreach { value =>
Review Comment:
The fix covers more than the spill path. On `main`, a global aggregate over
the same struct fails with no spilling at all:
```sql
SELECT sort_array(collect_list(s)), sort_array(collect_set(s))
FROM (SELECT named_struct('flag', true, 'id', coalesce(_2, 0), 'value', _3)
AS s FROM tbl WHERE _1 < 8)
```
The final aggregate fails with `column types must match schema types,
expected List(Struct("flag": Boolean, ...)) but found List(Struct("flag":
non-null Boolean, ...)) at column index 0`. With this change it passes. That
query runs through the ungrouped `Accumulator` rather than the
`GroupsAccumulator`, so the grouped cases here don't guard it. Could we add an
ungrouped case next to these two? It doesn't need the memory settings.
--
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]
