Skip to content

[core] Fix remaining byte[] identity comparisons in collect and merge_map aggregators - #9870

Merged
JingsongLi merged 1 commit into
apache:masterfrom
thswlsqls:fix/collect-agg-byte-array-identity-comparisons
Sep 17, 2026
Merged

JingsongLi merged 1 commit into
apache:masterfrom
thswlsqls:fix/collect-agg-byte-array-identity-comparisons

Conversation

@thswlsqls

Copy link
Copy Markdown
Contributor

Purpose

fix #9869

  • needsEqualiser() and BinaryMapKeys.isBinary() only matched BINARY_STRING, so GEOMETRY/GEOGRAPHY (also byte[]) were compared by identity: distinct collect / merge_map kept duplicates.
  • FieldCollectAgg built its equaliser only for distinct arrays, so a non-distinct binary retract() removed nothing.
  • Fix: isBinary() matches BINARY, VARBINARY, GEOMETRY, GEOGRAPHY (the getBinary roots of createElementGetter) and needsEqualiser() reuses it; the equaliser depends on the element type alone, agg() gates de-duplication on distinct.
  • Follow-up to [core] Compare binary elements and keys by value in collect and merge_map #9249, which merged 3.5 hours after [core] Support geospatial data types #9251 added geospatial types and deferred the retract gap.
  • Non-distinct retract of constructed elements now uses the generated equaliser (FLOAT/DOUBLE via Float.compare), like distinct.

Tests

  • Added FieldAggregatorTest#testFieldCollectAggWithDistinctGeospatial, #testFieldMergeMapAggWithGeospatialKey, #testFieldMergeMapWithKeyTimeAggWithGeospatialKey, #testFieldCollectAggRetractWithBinary (fail without the fix), #testFieldCollectAggKeepsDuplicatesWithBinary (regression guard).
  • FieldAggregatorTest: 106 tests, 0 failures.
  • mvn -pl paimon-core clean install (checkstyle, spotless, enforcer, surefire) — 5441 tests, 0 failures; only PostgresqlCatalogTest (needs Docker) could not start locally.

…_map aggregators

apache#9249 made collect and merge_map compare binary elements and keys by
value, but it recognised byte[]-backed types by the BINARY_STRING family
only. GEOMETRY and GEOGRAPHY (apache#9251, merged the same day) are byte[] as
well and still fell through to identity equality, so a distinct collect
kept the same WKB value twice and merge_map kept one entry per occurrence
of the same key.

FieldCollectAgg also built its equaliser only for distinct arrays, so
retract() on a non-distinct binary array compared byte[] by identity and
never removed anything.

- BinaryMapKeys.isBinary: match BINARY, VARBINARY, GEOMETRY, GEOGRAPHY
  (the roots InternalArray.createElementGetter reads with getBinary)
- FieldCollectAgg.needsEqualiser: reuse BinaryMapKeys.isBinary
- FieldCollectAgg: build the equaliser from the element type alone and
  gate de-duplication in agg() on distinct

Generated-by: Claude Code
@JingsongLi

Copy link
Copy Markdown
Contributor

Good catch. Two distinct fixes here, both correct:

  • FieldCollectAgg built the equaliser only when distinct was true, but retract() needs content equality regardless of distinct, so binary elements accumulated through retract could not be found/removed. Building it whenever the element type needs it, and gating only the agg() de-duplication on distinct, is the right split.
  • Extending the byte[]-valued set to GEOMETRY/GEOGRAPHY is warranted: their roots carry only the PREDEFINED family, so the old BINARY_STRING family check missed them and their elements compared by identity. This also fixes FieldMergeMapAgg/FieldMergeMapWithKeyTimeAgg, which share BinaryMapKeys.

@JingsongLi
JingsongLi merged commit 81a5682 into apache:master Sep 17, 2026
16 of 18 checks passed
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.

[Bug] collect/merge_map still compare some byte[] elements and keys by identity after #9249

2 participants