mbutrovich commented on code in PR #4720:
URL: https://github.com/apache/datafusion-comet/pull/4720#discussion_r3582715043
##########
spark/src/main/scala/org/apache/spark/sql/comet/operators.scala:
##########
@@ -1643,6 +1643,21 @@ trait CometBaseAggregate {
}
}
+ // CollectList/CollectSet declare their buffer as BinaryType in Spark but
produce a native
Review Comment:
Isn't this block unreachable? The `missingCometProducer` guard at line 1632
already returns `None` for this case. A `PartialMerge` stage makes
`consumesBuffers` true, and `CollectList`/`CollectSet` are always in
`aggsNotSupportingMixedExecution` since they don't override
`supportsMixedPartialFinal`. Looks like it predates the merge that brought that
guard over from `main`. Can we drop it?
##########
spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala:
##########
@@ -871,6 +871,24 @@ case class CometExecRule(session: SparkSession)
}
}
}
+
+ // CollectList/CollectSet round-trip an ArrayType buffer that Spark
declares as BinaryType.
Review Comment:
Could you add a comment on why this is separate from the tagging block just
above? They look like duplicates. The difference is that the block above only
tags when the Final can't convert, and `canAggregateBeConverted` skips the
child-native check, so an all-native `collect_list` Final converts and slips
past it. This block is what catches that all-native distinct case, which is the
Parquet half of the new suite test.
##########
spark/src/main/spark-4.2/org/apache/comet/shims/CometCollectShim.scala:
##########
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.comet.shims
+
+import org.apache.spark.sql.catalyst.expressions.aggregate.{CollectList,
CollectSet}
+
+/**
+ * Shim for the `ignoreNulls` flag on `CollectList` / `CollectSet`. Spark 4.2
added the field
+ * (`ignoreNulls = false` via `RESPECT NULLS`), so this shim reports the
actual value. The serde
+ * falls back to Spark when it is `false`, since the native path always drops
nulls.
+ */
+object CometCollectShim {
Review Comment:
These four files (3.4 through 4.1) are identical and only 4.2 differs. Could
the 3.4/3.5 pair share the existing `spark-3.x` dir the way `CometTypeShim`
does, so this is one fewer copy to keep in sync?
##########
spark/src/test/resources/sql-tests/expressions/aggregate/collect_list.sql:
##########
@@ -0,0 +1,405 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
+-- collect_list result order is non-deterministic across partitions, so
+-- every query wraps the result in sort_array to make comparisons stable.
+
+-- ============================================================
+-- Setup: tables
+-- ============================================================
+
+statement
+CREATE TABLE cl_src_int(i int, grp string) USING parquet
+
+statement
+INSERT INTO cl_src_int VALUES
+ (1, 'a'), (2, 'a'), (1, 'a'), (3, 'a'),
+ (4, 'b'), (4, 'b'), (NULL, 'b'), (5, 'b'),
+ (NULL, 'c'), (NULL, 'c')
+
+statement
+CREATE TABLE cl_src_nulls(val int, grp string) USING parquet
+
+statement
+INSERT INTO cl_src_nulls VALUES
Review Comment:
Could we add a `collect_list(x) FILTER (WHERE ...)` case, and something that
exercises a map input? The map path hits the `Map` arm in
`make_all_fields_nullable` that nothing covers yet. `sort_array` won't order a
map, so it'd need `spark_answer_only` or a size check.
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -439,6 +440,22 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
}
}
+ /**
+ * Returns true if any aggregate function produces a native intermediate
buffer whose Arrow type
+ * (e.g. ArrayType for CollectList/CollectSet) differs from the BinaryType
that Spark declares
Review Comment:
The comment describes this as matching any agg with a native `ArrayType`
buffer, but it only matches the collect functions. `Percentile` has the same
buffer shape (see `adjustOutputForNativeState`) and isn't matched. It's fine in
practice since `percentile` with distinct already passes, but the comment reads
broader than the code. Worth narrowing the comment, or inlining the match since
it's only used in one place now.
--
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]
