Repository navigation
Fix Row equality/hashing, RowUtils validation and ByteBuffer handling, and LogicalTypeCoder DATETIME conversion - #40789
sjvanrossum wants to merge 10 commits into
Conversation
… RowWithGetters equals/hashCode
…ls/hashCode contract
… SchemaCoderHelpers
There was a problem hiding this comment.
📋 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, andRowCoderTestthoroughly exercises all four fixes.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment |
| } | ||
| retValue = new RowWithStorage(schema, values); | ||
| } | ||
| if (retValue != null && !schema.equals(retValue.getSchema())) { |
There was a problem hiding this comment.
Was this added for a reason? It will add extra cost here.
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(getSchema(), getterTarget); | ||
| return super.hashCode(); |
There was a problem hiding this comment.
What's the reason for this change here?
| Schema.LogicalType<Object, Object> logicalType = | ||
| (Schema.LogicalType<Object, Object>) fieldType.getLogicalType(); | ||
| return deepHashCode( | ||
| SchemaUtils.toLogicalBaseType(logicalType, a), logicalType.getBaseType()); |
There was a problem hiding this comment.
why not just logicalType.toBaseType(a) instead of the SchemaUtils call?
| } else if (fieldType.getTypeName() == TypeName.LOGICAL_TYPE) { | ||
| return deepHashCode(a, fieldType.getLogicalType().getBaseType()); | ||
| Schema.LogicalType<Object, Object> logicalType = | ||
| (Schema.LogicalType<Object, Object>) fieldType.getLogicalType(); |
There was a problem hiding this comment.
Looking at the code it looks like we usually store input values in the row, meaning that this code is correct. However I'm not sure if we are consistent - the comment on attachValues says that base values should be passed. We should investigate this a bit more.
Fixes four correctness bugs across
Row,RowWithGetters,RowUtils, andSchemaCoderHelpers.LogicalTypeCoder.Row.Equals.deepHashCodepreviously passed the logical input value directly when recursing onfieldType.getLogicalType().getBaseType()without converting to the logical type's base value viaSchemaUtils.toLogicalBaseType(logicalType, a). When the base type wasBYTES(or a collection/map) and the input type was notbyte[](e.g.ByteBuffer),deepHashCodethrew aClassCastExceptioneven thoughRow.Equals.deepEqualsconverted to the base type first.RowWithGetters.equalsonly returnedtruewhen comparing twoRowWithGettersinstances ifObjects.equals(getterTarget, other.getterTarget)held, failing when the backing POJO/Bean did not overrideObject.equals, andRowWithGetters.hashCodehashed(getSchema(), getterTarget)instead of the row's field values, violating theObject.hashCodecontract whenever aRowWithGetterswas compared against an equivalentRowWithStorage.RowUtils.FieldValueBuilder.processRowpreviously returned an existingRowvalue without checking whether its schema matched the expected field schema, allowingRows with mismatched nested schemas (both inROWfields and inside collections/maps ofROW) to be constructed when field verification was bypassed or used viaFieldValueBuilder.RowUtilspreviously called((ByteBuffer) value).array()directly when convertingByteBuffervalues forBYTESfields. This threwUnsupportedOperationExceptionon read-only or directByteBuffers and silently ignoredposition(),limit(), andarrayOffset()on sliced/wrapped buffers, returning the entire backing array instead of the buffer's active slice.LogicalTypeCoder.encodeconvertedReadableInstantbase values toorg.joda.time.Instant(as required byInstantCoder), butstructuralValue,isRegisterByteSizeObserverCheap, andregisterByteSizeObservercalledlogicalType.toBaseType(value)directly without convertingReadableInstanttoInstant(and without guarding againstnull), causing aClassCastExceptioninInstantCoder.