Repository navigation
Fix IcebergIO rejecting timestamp strings with a space separator - #40861
mananmangal wants to merge 1 commit into
Conversation
1e88658 to
6a4ea25
Compare
There was a problem hiding this comment.
📋 Review Summary
This PR updates IcebergUtils.getIcebergTimestampValue to accept space-separated RFC 3339 timestamp strings when converting Beam STRING values to Iceberg timestamp and timestamptz columns, avoiding exception-based fallback overhead by checking the separator character up front. The implementation and unit test coverage are clean and well-targeted.
🔍 General Feedback
- One minor consistency detail to watch out for:
DateTimeFormatterBuilder.toFormatter()defaults toResolverStyle.SMART, whereasLocalDateTime.parse(ISO_LOCAL_DATE_TIME) andOffsetDateTime.parse(ISO_OFFSET_DATE_TIME) useResolverStyle.STRICT.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
Accept a space between date and time when writing a STRING value to a timestamp or timestamptz column, as allowed by RFC 3339 section 5.6. Fixes apache#40860
6a4ea25 to
f7586e9
Compare
|
/gemini review |
There was a problem hiding this comment.
📋 Review Summary
This PR updates IcebergUtils to support space-separated ISO-8601/RFC-3339 timestamp strings when writing STRING fields to Iceberg timestamp and timestamptz columns. The previous review feedback regarding strict date resolution (ResolverStyle.STRICT and IsoChronology.INSTANCE) has been properly addressed, and unit tests cover both valid and invalid date/separator cases.
🔍 General Feedback
- The up-front
hasSpaceSeparatorcheck avoids exception-handling overhead on the hot path while preserving strict parsing semantics matchingDateTimeFormatter.ISO_LOCAL_DATE_TIMEandDateTimeFormatter.ISO_OFFSET_DATE_TIME. - Test coverage in
IcebergUtilsTestthoroughly validates space-separated timestamps, invalid separators, and invalid calendar dates across bothtimestampandtimestamptztypes.
💡 Tip: Comment /gemini review on this pull request to trigger a new Gemini code review.
|
Assigning reviewers: R: @ahmedabu98 for label iceberg. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40861 +/- ##
===============================
===============================
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Fixes #40860
IcebergIO only accepted a
Tbetween date and time when writing aSTRINGvalue to atimestamportimestamptzcolumn. A space, which RFC 3339 section 5.6 allows and Python'sstr(datetime)produces, threwDateTimeParseException.When the character after the date is a space, the value is now parsed with a formatter that expects a space instead of
T. All other values go through the existingOffsetDateTime.parse/LocalDateTime.parsecalls unchanged.2024-10-08 T13:18:20(the output behind [Bug]: BigQueryIO Storage Write API failed-rows output crashes: TIMESTAMP printed with both a space and a T separator, then parsed by Instant.parse #40110) is still rejected.Testing: added cases to
IcebergUtilsTestfor both column types, including invalid separators.:sdks:java:io:iceberg:testpasses.