kosiew commented on code in PR #23483:
URL: https://github.com/apache/datafusion/pull/23483#discussion_r3575920007
##########
datafusion/sqllogictest/test_files/dictionary.slt:
##########
@@ -632,4 +632,61 @@ south 2
statement ok
DROP TABLE dict_count_distinct;
+# overlapping values with different key assignments across batches
+query TI rowsort
+WITH
+ first_batch AS (
+ SELECT arrow_cast(column1, 'Dictionary(Int32, Utf8)') AS region
+ FROM (VALUES ('west'), ('east'), ('west'), (NULL)) AS t(column1)
+ ),
+ second_batch AS (
+ SELECT arrow_cast(column1, 'Dictionary(Int16, LargeUtf8)') AS region
Review Comment:
I don't think these new regression cases are exercising dictionary grouping
at the aggregation boundary.
Because the UNION inputs use different dictionary key/value types, for
example `Dictionary(Int32, Utf8)` unioned with `Dictionary(Int16, LargeUtf8)`
here, the physical plan coerces both branches back to `LargeUtf8` before
`UnionExec` and `AggregateExec`: `CAST(CAST(column1 AS Dictionary(...)) AS
LargeUtf8)`.
That means the `GROUP BY` sees plain strings, so an implementation that
incorrectly hashed dictionary key ids would still pass these tests.
Could you construct the inputs using the same dictionary type while varying
the dictionary value order instead? Alternatively, if there's another approach,
could you verify through the plan or test setup that `AggregateExec` is
actually consuming separate dictionary arrays with different dictionaries?
##########
datafusion/sqllogictest/test_files/dictionary.slt:
##########
@@ -632,4 +632,61 @@ south 2
statement ok
DROP TABLE dict_count_distinct;
+# overlapping values with different key assignments across batches
Review Comment:
Once the construction is fixed, I think one focused cross input dictionary
key regression test would be enough here. A short comment noting that the plan
was checked to preserve the dictionary type through the UNION would also make
the intent clear. The current three cases are quite similar, and a single
targeted case would be a bit easier to maintain.
--
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]
