github-actions[bot] commented on code in PR #41027:
URL: https://github.com/apache/beam/pull/41027#discussion_r4486028421
##########
sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/IcebergUtils.java:
##########
@@ -94,25 +95,49 @@ private IcebergUtils() {}
.put(EnumerationType.IDENTIFIER, Types.StringType.get())
.build();
- private static final DateTimeFormatter TIMESTAMP_SPACE_FORMATTER =
+ /**
+ * Parses Iceberg {@code timestamp} strings: an ISO-8601 date and time,
optionally separated by a
+ * space, a {@code 'T'}, or both, e.g. {@code 2024-10-08T13:18:20.053} or
{@code 2024-10-08
+ * 13:18:20.053}.
+ */
+ private static final DateTimeFormatter TIMESTAMP_FORMATTER =
new DateTimeFormatterBuilder()
.parseCaseInsensitive()
.append(ISO_LOCAL_DATE)
+ .optionalStart()
.appendLiteral(' ')
+ .optionalEnd()
+ .optionalStart()
+ .appendLiteral('T')
+ .optionalEnd()
.append(ISO_LOCAL_TIME)
.toFormatter()
.withResolverStyle(ResolverStyle.STRICT)
.withChronology(IsoChronology.INSTANCE);
- private static final DateTimeFormatter TIMESTAMPTZ_SPACE_FORMATTER =
+ /**
+ * Parses Iceberg {@code timestamptz} strings: {@link #TIMESTAMP_FORMATTER},
then an optional
+ * offset, then an optional space and zone ID, e.g. {@code
2024-10-08T13:18:20.053Z}, {@code
+ * 2024-10-08 13:18:20.053+03:27} or {@code 2024-10-08 13:18:20.053 UTC}. If
both are present, the
+ * offset takes precedence. Without either, UTC is assumed.
+ */
+ private static final DateTimeFormatter TIMESTAMPTZ_FORMATTER =
new DateTimeFormatterBuilder()
- .append(TIMESTAMP_SPACE_FORMATTER)
+ .append(TIMESTAMP_FORMATTER)
+ .optionalStart()
Review Comment:
**🟢 Nit**: `testTimestampWithZone()` asserts that an offset preceded by a
space (e.g., `"2024-10-08 13:18:20.053 +03:27"`) is accepted, but
`TIMESTAMPTZ_FORMATTER` does not include an optional space before
`appendOffsetId()`. This works only when `.optionalStart()` precedes
`.appendLiteral(' ')` (or because the second optional block matches the space
and fails `appendZoneRegionId()`, which would fail if not for how
`optionalStart` backtracks—actually, in `"2024-10-08 13:18:20.053 +03:27"`,
`appendOffsetId()` does not consume the space, and `appendZoneRegionId()` in
the second optional block actually parses `"+03:27"` as a `ZoneId`!). Relying
on `appendZoneRegionId()` to parse offset strings after a space is subtle and
means `"2024-10-08 09:51:20.053 Z"` (space before `Z`) would fail because
`appendZoneRegionId()` rejects `"Z"`. Consider adding an optional space before
`appendOffsetId()` if space-prefixed offsets are intended to be supported
consistently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
