Skip to content

feat: native MAX_BY and MIN_BY with a string value - #14

Closed
msafonov wants to merge 1 commit into
joom/1.1-native-string-minmaxfrom
joom/1.1-native-string-maxby
Closed

msafonov wants to merge 1 commit into
joom/1.1-native-string-minmaxfrom
joom/1.1-native-string-maxby

Conversation

@msafonov

Copy link
Copy Markdown
Collaborator

Depends on #12. The base branch is #12's branch; merge this right after #12.

What

  • MAX_BY/MIN_BY accept a StringType value with the default UTF8_BINARY collation. The ordering is still limited to fixed-length types; a string ordering, a collated value and other variable-length types stay in Spark.
  • No native change. MaxMinBy (both the single and the groups accumulator) stores the value as Arrow row bytes and only compares the ordering. Its native unit tests already use a Utf8 value.
  • A string value makes Spark plan a SortAggregate. With feat: native MIN/MAX over strings and SortAggregate as a native hash aggregate #12 it is converted when its other aggregates are order-insensitive, and MaxMinBy now counts as such.

Semantics vs Spark

  • Nulls: a null ordering is ignored. If the extremum ordering carries a null value, that null is returned. A group whose orderings are all null gives null.
  • Ties: Spark's update keeps the later row (the predicate is strict old > new, or < for min_by), and its merge keeps the incoming buffer. Which tied row wins therefore depends on input and merge order, and Spark's documentation calls max_by non-deterministic. The native accumulator also keeps the later row of a batch, but it may still pick a different tied row than Spark. This is the same difference the existing native path for fixed-length values already documents in getCompatibleNotes. Compares should not expect equality on rows tied on the ordering.
  • Unlike first/last: max_by/min_by depend on the input order only among tied rows. FIRST/LAST/ANY_VALUE stay in Spark per the feat: native MIN/MAX over strings and SortAggregate as a native hash aggregate #12 fix.

Tests

  • CometAggregateSuite:
    • max_by/min_by over strings with nulls, an empty string, non-ASCII text and null orderings; grouped and ungrouped; AQE on and off (partial/final). The SortAggregate is gone and results equal Spark.
    • Ties: the result is one of the tied values.
    • Native partial with a Spark final.
    • An ungrouped order-sensitive aggregate over ordered input stays in Spark.
  • max_by.sql/min_by.sql: the string-value fallback expectations now run natively, plus new string-value queries. The string-ordering fallbacks are unchanged.
  • Spark 3.5:
    • CometAggregateSuite, CometSqlFileTestSuite, CostBasedEngineChoiceSuite, WideRowSortFallbackSuite, CometExecSuite, TPC-DS V1_4/V2_7 plan stability: 1069 passed, 0 failed.
    • CometFirstLastBoolAggFuzzSuite full: 31/31.
  • Spark 4.1: CometFirstLastBoolAggFuzzSuite (full), CometAggregateSuite, CometSqlFileTestSuite: 741 passed, 0 failed.
  • spotless clean. No new suites.

🤖 Generated with Claude Code

MAX_BY and MIN_BY accept a StringType value with the default UTF8_BINARY
collation; the ordering stays limited to fixed-length types. The native
MaxMinBy accumulators already store the value as Arrow row bytes and only
compare the ordering, so no native change is needed.

A string value makes Spark plan a SortAggregate, which is now converted
when its other aggregates are order-insensitive. MAX_BY and MIN_BY depend on
the input order only among rows tied on the ordering, where Spark is
non-deterministic too and the native path already documents that it may pick
a different row.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added enhancement New feature or request area:aggregation labels Oct 10, 2026
msafonov added a commit that referenced this pull request Oct 10, 2026
@msafonov

Copy link
Copy Markdown
Collaborator Author

Merged into branch-1.1 via #15 (3014b8e).

@msafonov msafonov closed this Oct 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:aggregation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant