andygrove commented on code in PR #6927:
URL: https://github.com/apache/datafusion-comet/pull/6927#discussion_r4261304639
##########
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:
Nit, optional: two more shapes go native with this change that the file
doesn't cover. `count(DISTINCT i)` next to `min(s)`/`max(b)` runs as four
native sort aggregates, including a `PartialMerge`, so `MergeAsPartial` now
wraps DataFusion's `max` over Utf8 and Binary state. A `VALUES` input keeps the
partial in Spark and runs the final natively through
`supportsSparkPartialToNativeFinal`. Both matched Spark when I tried them on
3.4 through 4.2. Could we add a query for each? `partial_merge.sql` already has
a string column in `pm_basic`, and the `VALUES` one would need
`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:
Nit, optional: `CometMin` and `CometMax` don't override
`getUnsupportedReasons`, so the generated compatibility page won't say that a
collated string falls back, or mention the timestamp-NTZ, interval and nested
cases. Could we add one, as `CometAverage` and `CometMaxMinBy` do?
--
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]
