comphead commented on code in PR #6927:
URL: https://github.com/apache/datafusion-comet/pull/6927#discussion_r4263845137
##########
spark/src/test/resources/sql-tests/expressions/aggregate/min_max.sql:
##########
@@ -15,17 +15,49 @@
-- specific language governing permissions and limitations
-- under the License.
+-- Run with and without Parquet dictionary encoding, which changes how string
and binary columns
+-- are read.
+-- ConfigMatrix: parquet.enable.dictionary=false,true
+
statement
CREATE TABLE test_min_max(i int, d double, s string, grp string) USING parquet
statement
INSERT INTO test_min_max VALUES (1, 1.5, 'b', 'x'), (3, 3.5, 'a', 'x'), (2,
2.5, 'c', 'y'), (NULL, NULL, NULL, 'y'), (-1, -1.5, 'z', 'x')
--- min/max over StringType is not yet supported natively, so this falls back
to Spark.
--- (Spark plans this global aggregate as SortAggregateExec because the string
min/max buffer
--- is not hash-friendly; that operator is now supported, but the string
aggregate itself is not.)
-query expect_fallback(Unsupported data type: StringType)
+-- Spark plans this as SortAggregateExec, because a string min/max buffer is
not hash-friendly.
+query
SELECT min(i), max(i), min(d), max(d), min(s), max(s) FROM test_min_max
query
SELECT grp, min(i), max(i) FROM test_min_max GROUP BY grp ORDER BY grp
+
+-- Spark compares strings and binary as unsigned bytes, and a value sorts
before any longer value
+-- it is a prefix of. The groups cover prefixes, case, the empty value,
characters whose UTF-8
+-- order differs from their UTF-16 order ('😀' sorts after 'ｱ' by bytes but
before it by UTF-16
+-- code units), bytes that a signed comparison would order the other way
(X'80' after X'7F'),
+-- and a group of only NULLs.
+statement
+CREATE TABLE test_min_max_bytes(grp string, s string, b binary) USING parquet
+
+statement
+INSERT INTO test_min_max_bytes VALUES
+ ('prefix', 'ab', X'0102'), ('prefix', 'a', X'01'), ('prefix', NULL, NULL),
+ ('case', 'a', X'61'), ('case', 'B', X'42'), ('case', 'A', X'41'),
+ ('empty', ' ', X'20'), ('empty', '', X''), ('empty', NULL, NULL),
+ ('utf8', 'ｱ', X'7F'), ('utf8', '😀', X'80'), ('utf8', 'é', X'FF'), ('utf8',
'z', X'00'),
+ ('nulls', NULL, NULL), ('nulls', NULL, NULL)
+
+query
+SELECT grp, min(s), max(s), min(b), max(b) FROM test_min_max_bytes GROUP BY
grp ORDER BY grp
+
+query
+SELECT min(s), max(s), min(b), max(b) FROM test_min_max_bytes
+
+query
+SELECT min(s), max(s), min(b), max(b) FROM test_min_max_bytes WHERE grp =
'missing'
+
+-- collect_set makes Spark plan ObjectHashAggregateExec instead, with min/max
in the same operator.
+query
+SELECT grp, min(s), max(b), sort_array(collect_set(s))
+FROM test_min_max_bytes GROUP BY grp ORDER BY grp
Review Comment:
Added both in 3752f31136. The `count(DISTINCT)` query went into
`min_max.sql` rather than `partial_merge.sql`, because `test_min_max_bytes` has
a binary column and the byte-order cases, so the `PartialMerge` covers both
Utf8 and Binary state. The `VALUES` one is `spark_answer_only`.
##########
spark/src/main/scala/org/apache/comet/serde/aggregates.scala:
##########
@@ -1475,14 +1475,16 @@ object AggSerde {
/**
* Shared support level for `Min` / `Max` based on the result data type.
Floats follow Spark's
- * ordering natively, so strict floating-point mode does not apply to them.
+ * ordering natively, so strict floating-point mode does not apply to them.
Spark and the native
+ * accumulators both compare strings and binary as unsigned bytes, except
that Spark orders a
+ * string with a non-default collation by that collation.
*/
- def minMaxSupportLevel(dt: DataType): SupportLevel = {
- if (!minMaxDataTypeSupported(dt)) {
- Unsupported(Some(s"Unsupported data type: $dt"))
- } else {
- Compatible()
- }
+ def minMaxSupportLevel(dt: DataType): SupportLevel = dt match {
Review Comment:
Added in 3752f31136. `CometMin` and `CometMax` now share
`AggSerde.minMaxUnsupportedReasons`, which lists the unsupported input types
(timestamp without time zone, intervals, arrays and structs among them) and the
collated-string case.
--
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]
