Repository navigation
GH-551: [Java] Add TimeStamp*TZ support to ComplexCopier - #1414
henriquejsza wants to merge 1 commit into
Conversation
ComplexCopier skipped every minor type that has type parameters except Decimal, so copying a vector containing TimeStampSecTZ, TimeStampMilliTZ, TimeStampMicroTZ, or TimeStampNanoTZ threw UnsupportedOperationException. Generate the copy and writer-lookup cases for the timezone-aware timestamp types the same way as the other scalar types, and pass the timezone through to the struct writer so it is preserved. Add list, struct and map copy tests for TimeStampNanoTZ. Closes apache#551. Signed-off-by: henriquejsza <henriquejsza@gmail.com>
| <#assign fields = minor.fields!type.fields /> | ||
| <#assign uncappedName = name?uncap_first/> | ||
| <#if !minor.typeParams?? || minor.class?starts_with("Decimal") > | ||
| <#if !minor.typeParams?? || minor.class?starts_with("Decimal") || is_timestamp_tz(minor.class) > |
There was a problem hiding this comment.
Adding the TZ types to the map lookup turns an UnsupportedOperationException into silently wrong data. UnionMapWriter only overrides the key/value getters for types without typeParams (plus decimal, decimal256 and fixedSizeBinary). So writer.value().timeStampNanoTZ() falls through to UnionListWriter.timeStampNanoTZ(), which returns the map writer itself. The value is then written into the map entries vector instead of the value field, and the entries vector gets promoted to a union.
Copying into MapVector.empty(...) would give the same result.
We could either:
- drop the TZ case from
getMapWriterForReader, so maps keep throwing and the list/struct support can land on its own, or - add TZ overrides to
UnionMapWriterthat route to thekey/valuefields ofentryWriterand carry the timezone, as the struct path does?
| mapWriter.startMap(); | ||
| mapWriter.startEntry(); | ||
| mapWriter.key().integer().writeInt(i); | ||
| mapWriter.value().timeStampNanoTZ().writeTimeStampNanoTZ(i * 1_000_000L); |
There was a problem hiding this comment.
This test can't catch the problem mentioned above, because the source map is already malformed. mapWriter.value().timeStampNanoTZ() hits the same missing UnionMapWriter override, so from already has its entries promoted to a union and no value data. The comment at the top of the test blames the mismatch on timezones, but I think both sides are UTC here, the vectors differ in structure, not timezone.
Could we build the source directly on the vectors (startNewValue/endValue on the map, set key/value on the entries StructVector)? Then use a non-UTC zone and assert VectorEqualsVisitor.vectorEquals(from, to) instead of assertDoesNotThrow plus the value count. If we drop the map case instead, an assertThrows(UnsupportedOperationException.class, ...) would document that maps are not support yet 😄
What's Changed
ComplexCopierskipped every minor type that has type parameters exceptDecimal, so copying a vector containingTimeStampSecTZ,TimeStampMilliTZ,TimeStampMicroTZ, orTimeStampNanoTZthrewUnsupportedOperationException.This generates the copy and writer-lookup cases for the timezone-aware
timestamp types the same way as the other scalar types, and passes the
timezone through to the struct writer so it is preserved. The fix is in the
FreeMarker template
vector/src/main/codegen/templates/ComplexCopier.java;the generated
ComplexCopieris produced from it.Added list, struct, and map copy tests for
TimeStampNanoTZ.Closes #551.