gemini-code-assist[bot] commented on code in PR #36425:
URL: https://github.com/apache/beam/pull/36425#discussion_r2478555752
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/BigQueryUtils.java:
##########
@@ -169,11 +171,46 @@ public abstract static class Builder {
}
private static final String BIGQUERY_TIME_PATTERN = "HH:mm:ss[.SSSSSS]";
- private static final java.time.format.DateTimeFormatter
BIGQUERY_TIME_FORMATTER =
+ static final java.time.format.DateTimeFormatter BIGQUERY_TIME_FORMATTER =
java.time.format.DateTimeFormatter.ofPattern(BIGQUERY_TIME_PATTERN);
- private static final java.time.format.DateTimeFormatter
BIGQUERY_DATETIME_FORMATTER =
+ static final java.time.format.DateTimeFormatter BIGQUERY_DATETIME_FORMATTER =
java.time.format.DateTimeFormatter.ofPattern("uuuu-MM-dd'T'" +
BIGQUERY_TIME_PATTERN);
+ // Custom formatter that accepts "2022-05-09 18:04:59.123456"
+ // The old dremel parser accepts this format, and so does insertall. We need
to accept it
+ // for backwards compatibility, and it is based on UTC time.
+ static final java.time.format.DateTimeFormatter DATETIME_SPACE_FORMATTER =
+ new java.time.format.DateTimeFormatterBuilder()
+ .append(java.time.format.DateTimeFormatter.ISO_LOCAL_DATE)
+ .optionalStart()
+ .appendLiteral(' ')
+ .optionalEnd()
+ .optionalStart()
+ .appendLiteral('T')
+ .optionalEnd()
+ .append(java.time.format.DateTimeFormatter.ISO_LOCAL_TIME)
+ .toFormatter()
+ .withZone(ZoneOffset.UTC);
Review Comment:
The implementation of `DATETIME_SPACE_FORMATTER` can lead to incorrect
parsing. It allows datetime strings with no separator between date and time
(e.g., `2023-01-0112:00:00`) because both space and 'T' separators are made
optional by `optionalStart()`. A separator should be mandatory.
Given that `toBeamRow` tries `LocalDateTime.parse()` first (which handles
'T'), this formatter is likely intended for the space-separated case as a
fallback. Simplifying it would fix the bug. Note that this formatter is also
used by `TIMESTAMP_FORMATTER`, so the impact of this change should be
considered.
A simplified version that enforces a space separator would be:
```java
static final java.time.format.DateTimeFormatter DATETIME_SPACE_FORMATTER =
new java.time.format.DateTimeFormatterBuilder()
.append(java.time.format.DateTimeFormatter.ISO_LOCAL_DATE)
.appendLiteral(' ')
.append(java.time.format.DateTimeFormatter.ISO_LOCAL_TIME)
.toFormatter()
.withZone(ZoneOffset.UTC);
```
--
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]
