[common] Compare maps of different representations without throwing - #9211
Open
PDGGK wants to merge 1 commit into
Open
[common] Compare maps of different representations without throwing#9211PDGGK wants to merge 1 commit into
PDGGK wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Closes #9210.
InternalRowUtils.equalspicked theGenericMapfast path by testing data1, then cast data2 with no test of its own:so
equals(generic, binary, mapType)throws whileequals(binary, generic, mapType)returnstrue.Two things say this is unintended rather than a restriction:
elsebranch exists because the representation varies. OneMapTypeis carried byGenericMap,BinaryMaporColumnarMapinterchangeably; the conversion is there to normalise exactly that. Gating it on data1's concrete class contradicts the branch it guards.InternalRowUtils.hashalready 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
InternalRowUtilsTestasserting three things:hash(generic) == hash(binary)equals(binary, generic)equals(generic, binary)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.
Related suites pass (
InternalRowSerializerTest,RowCompactedSerializerTest).spotless:checkandcheckstyle:checkexit 0.