ajantha-bhat commented on code in PR #11904:
URL: https://github.com/apache/iceberg/pull/11904#discussion_r1925697904
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/BaseParquetWriter.java:
##########
@@ -192,13 +222,17 @@ public Optional<ParquetValueWriters.PrimitiveWriter<?>>
visit(
@Override
public Optional<ParquetValueWriters.PrimitiveWriter<?>> visit(
LogicalTypeAnnotation.DateLogicalTypeAnnotation dateType) {
- return Optional.of(new DateWriter(desc));
+ return Optional.ofNullable(dateWriter(desc));
}
@Override
public Optional<ParquetValueWriters.PrimitiveWriter<?>> visit(
LogicalTypeAnnotation.TimeLogicalTypeAnnotation timeType) {
- return Optional.of(new TimeWriter(desc));
+ Preconditions.checkArgument(
+ LogicalTypeAnnotation.TimeUnit.MICROS.equals(timeType.getUnit()),
+ "Cannot write time in %s, only MICROS is supported",
+ timeType.getUnit());
+ return Optional.ofNullable(timeWriter(desc));
Review Comment:
Because the `InternalWriter` returns null for `timewriter()` meaning use the
primitive writer (no need of special handling). Existing `Optional.of()` will
give NPE. To avoid it, I used `Optional.ofNullable()` so it returns
`Optional.empty()` when it is null.
It was also the reason why I was returning optional from the default
implementation signatures in
https://github.com/apache/iceberg/pull/11904#discussion_r1917303018.
I will do the null check manually and return super method and revert back to
`Optional.of()`, if it looks odd.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
