Skip to content

Fix Row equality/hashing, RowUtils validation and ByteBuffer handling, and LogicalTypeCoder DATETIME conversion - #40789

Open
sjvanrossum wants to merge 10 commits into
apache:masterfrom
sjvanrossum:row-coder-bug
Open

sjvanrossum wants to merge 10 commits into
apache:masterfrom
sjvanrossum:row-coder-bug

Conversation

@sjvanrossum

@sjvanrossum sjvanrossum commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Fixes four correctness bugs across Row, RowWithGetters, RowUtils, and SchemaCoderHelpers.LogicalTypeCoder.

Row.Equals.deepHashCode previously passed the logical input value directly when recursing on fieldType.getLogicalType().getBaseType() without converting to the logical type's base value via SchemaUtils.toLogicalBaseType(logicalType, a). When the base type was BYTES (or a collection/map) and the input type was not byte[] (e.g. ByteBuffer), deepHashCode threw a ClassCastException even though Row.Equals.deepEquals converted to the base type first. RowWithGetters.equals only returned true when comparing two RowWithGetters instances if Objects.equals(getterTarget, other.getterTarget) held, failing when the backing POJO/Bean did not override Object.equals, and RowWithGetters.hashCode hashed (getSchema(), getterTarget) instead of the row's field values, violating the Object.hashCode contract whenever a RowWithGetters was compared against an equivalent RowWithStorage.

RowUtils.FieldValueBuilder.processRow previously returned an existing Row value without checking whether its schema matched the expected field schema, allowing Rows with mismatched nested schemas (both in ROW fields and inside collections/maps of ROW) to be constructed when field verification was bypassed or used via FieldValueBuilder.

RowUtils previously called ((ByteBuffer) value).array() directly when converting ByteBuffer values for BYTES fields. This threw UnsupportedOperationException on read-only or direct ByteBuffers and silently ignored position(), limit(), and arrayOffset() on sliced/wrapped buffers, returning the entire backing array instead of the buffer's active slice.

LogicalTypeCoder.encode converted ReadableInstant base values to org.joda.time.Instant (as required by InstantCoder), but structuralValue, isRegisterByteSizeObserverCheap, and registerByteSizeObserver called logicalType.toBaseType(value) directly without converting ReadableInstant to Instant (and without guarding against null), causing a ClassCastException in InstantCoder.

@github-actions github-actions Bot added java core size/m M: 50-249 lines changed labels Oct 25, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📋 Review Summary

This PR fixes four distinct correctness issues across Row, RowWithGetters, RowUtils, and SchemaCoderHelpers.LogicalTypeCoder, ensuring proper equality/hashing contracts, nested Row schema validation, ByteBuffer slice/read-only conversion, and DATETIME logical type base conversions.

🔍 General Feedback

  • The fixes are clean, minimal, and directly address the root causes of each bug.
  • Unit test coverage across RowTest, JavaFieldSchemaTest, and RowCoderTest thoroughly exercises all four fixes.

💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.

@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

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

Labels

core java size/m M: 50-249 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant