Skip to content

[common] Compare maps of different representations without throwing - #9211

Open
PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:fix-map-equals-mixed-impl
Open

[common] Compare maps of different representations without throwing#9211
PDGGK wants to merge 1 commit into
apache:masterfrom
PDGGK:fix-map-equals-mixed-impl

Conversation

@PDGGK

@PDGGK PDGGK commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Purpose

Closes #9210.

InternalRowUtils.equals picked the GenericMap fast path by testing data1, then cast data2 with no test of its own:

if (data1 instanceof GenericMap) {
    map1 = (GenericMap) data1;
    map2 = (GenericMap) data2;      // BinaryMap -> ClassCastException

so equals(generic, binary, mapType) throws while equals(binary, generic, mapType) returns true.

Two things say this is unintended rather than a restriction:

  • The else branch exists because the representation varies. One MapType is carried by GenericMap, BinaryMap or ColumnarMap interchangeably; the conversion is there to normalise exactly that. Gating it on data1's concrete class contradicts the branch it guards.
  • InternalRowUtils.hash already treats them as interchangeable, returning the same value for both. The class says "equal" by hash and throws when asked directly.

Each operand is now converted on its own, keeping the fast path when both are already GenericMap.

Scope, deliberately narrow. The other casts in this method — (InternalRow) data2, (InternalArray) data2 — look like the same shape but are interface casts, and every representation implements those interfaces, so they cannot fail this way. Only the map branch casts to a concrete class. I have left them alone.

Tests

One case in InternalRowUtilsTest asserting three things:

assertion before
hash(generic) == hash(binary) green — this is what pins the intent
equals(binary, generic) green
equals(generic, binary) ClassCastException

Only the third changes. The other two being green both before and after is the point: they establish that the two representations were already meant to be equivalent, so the third is a defect rather than a new feature.

InternalRowUtilsTest ... ClassCastException: class BinaryMap cannot be cast to class GenericMap
Tests run: 7, Failures: 0, Errors: 1      <- master

Tests run: 7, Failures: 0, Errors: 0      <- with the fix

Related suites pass (InternalRowSerializerTest, RowCompactedSerializerTest). spotless:check and checkstyle:check exit 0.

InternalRowUtils.equals picked the GenericMap fast path by testing
data1, then cast data2 to GenericMap with no test of its own:

    if (data1 instanceof GenericMap) {
        map1 = (GenericMap) data1;
        map2 = (GenericMap) data2;    // BinaryMap -> ClassCastException

So comparing a GenericMap against a BinaryMap throws, while the reverse
ordering returns true. The conversion in the else branch exists
precisely because one MapType is represented by GenericMap, BinaryMap or
ColumnarMap interchangeably, so the operand-1 gate contradicts the
branch it guards.

InternalRowUtils.hash already treats the two as interchangeable and
returns the same value for both, so equals throwing for one ordering is
an inconsistency inside a single class rather than a deliberate
restriction.

Each operand is now converted on its own, keeping the fast path when
both are already GenericMap.

The other casts in this method -- (InternalRow) data2, (InternalArray)
data2 -- are interface casts and are not affected: every representation
implements them.

Test asserts both orderings and the hash agreement. Only the
generic-first ordering changes; the other two are green before and
after, which is what pins the intent rather than just the fix.
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] InternalRowUtils.equals throws ClassCastException when comparing a GenericMap with a BinaryMap

1 participant