Skip to content

[SPARK-58951][SQL] Support non-binary collations in collect_set - #58230

Open
vinodkc wants to merge 2 commits into
apache:masterfrom
vinodkc:fix-collect-set-collation
Open

[SPARK-58951][SQL] Support non-binary collations in collect_set#58230
vinodkc wants to merge 2 commits into
apache:masterfrom
vinodkc:fix-collect-set-collation

Conversation

@vinodkc

@vinodkc vinodkc commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This PR makes collect_set support non-binary collated string inputs . Previously such inputs were rejected with an UNSUPPORTED_INPUT_TYPE error; now collect_set deduplicates them collation-aware, returning one representative per collation-equal group. Binary-stable inputs are unchanged, and MapType is still rejected.

Why are the changes needed?

collect_set on a non-binary collated column throws instead of working:

CREATE OR REPLACE TEMP VIEW v AS
SELECT CAST(s AS STRING COLLATE UTF8_LCASE) AS s
FROM VALUES ('foo'), ('FOO'), ('bar') AS raw(s);

SELECT collect_set(s) FROM v;
-- [DATATYPE_MISMATCH.UNSUPPORTED_INPUT_TYPE] ... collect_set ...

Under UTF8_LCASE, 'foo' and 'FOO' are equal, so collect_set should treat them as one group rather than error out. This closes a gap in collation support for aggregate functions.

Actual result after this PR:

SELECT collect_set(s) FROM v;
+----------------+
|collect_set(s)  |
+----------------+
|[foo, bar]      |
+----------------+

Does this PR introduce any user-facing change?

Yes. collect_set over a non-binary collated string (top-level or nested in a struct/array) now succeeds instead of raising UNSUPPORTED_INPUT_TYPE, returning one representative per collation-equal group. For the view above, the result has 2 elements ({foo-group, bar}). As with any set aggregate, which representative of a group appears is unspecified.
No change for default (UTF8_BINARY) or non-string inputs; MapType is still rejected.

How was this patch tested?

Added new tests in CollationAggregationSuite

Was this patch authored or co-authored using generative AI tooling?

Yes. Generated-by: Claude Code

Co-authored-by: Isaac <no-reply@databricks.com>

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking into this

@uros-b uros-b left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note same gaps in test coverage: the new path is exercised only at nesting depth 1 and only for case-folding collations (UTF8_LCASE, UNICODE_CI). Two cheap additions would close the confidence gaps in the injectCollationKey recursion this PR now depends on: (1) a depth≥2 case, e.g. collect_set(array(named_struct('s', ))); and (2) a space-trimming collation like UTF8_LCASE_RTRIM, where 'foo ' and 'foo' must fold to one key.

@uros-b

uros-b commented Aug 23, 2026

Copy link
Copy Markdown
Member

Other than the above, the changes seem correct and cleanly scoped - thank you @vinodkc!

@vinodkc
vinodkc force-pushed the fix-collect-set-collation branch from 74b5741 to 7e0d2ac Compare August 24, 2026 04:59
@vinodkc

vinodkc commented Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @uros-b.
Added both tests in CollationAggregationSuite

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants