vbhanuchander-lang opened a new pull request, #17613:
URL: https://github.com/apache/iceberg/pull/17613
Part of #15443.
### The gap
`RecordConverter` has no `case TIMESTAMP_NANO`, so writing to a table with a
`timestamp_ns` column through the Kafka Connect sink fails outright, while
Spark handles the same column:
```
UnsupportedOperationException: Unsupported type: timestamp_ns
```
The generic object model already supports the type, so the sink was the only
thing missing:
```java
// GenericDataUtil.java:54
case TIMESTAMP_NANO:
if (((Types.TimestampNanoType) type).shouldAdjustToUTC()) {
return DateTimeUtil.timestamptzFromNanos((Long) value);
} else {
return DateTimeUtil.timestampFromNanos((Long) value);
}
```
### The change
The Java representations for `timestamp_ns` are the same as for `timestamp`
— `OffsetDateTime` and `LocalDateTime` — so `convertTimestampNanoValue` mirrors
`convertTimestampValue` and reuses the existing converters. Both types carry
nanoseconds and the ISO parser accepts them, so precision survives for string
and temporal inputs without any new parsing code.
### The one decision worth reviewing
**A numeric input stays milliseconds.** `convertOffsetDateTime` /
`convertLocalDateTime` do `millis * 1000`, which is the Kafka Connect
`Timestamp` logical type convention, and it is what `case TIMESTAMP` already
does in the same switch.
I chose consistency over matching the column's unit, for two reasons:
Connect's `Timestamp` is millisecond-based and cannot express finer precision
anyway, and having two adjacent cases in one switch disagree about what a bare
number means seemed worse than the theoretical loss. Nanosecond data
realistically arrives as an ISO string or a temporal, both of which keep full
precision here.
It is pinned by a test rather than left implicit, because reading a number
as nanoseconds instead would shift every such value by six orders of magnitude
**without failing**:
```java
Temporal nanoVal = converter.convertTimestampNanoValue(millis,
TimestampNanoType.withZone());
Temporal microVal = converter.convertTimestampValue(millis,
TimestampType.withZone());
assertThat(nanoVal).as("must agree with the timestamp
path").isEqualTo(microVal);
```
If you would rather a numeric were interpreted as nanoseconds for this type,
it is a small change and I am happy to make it — I would just want it to be a
deliberate choice rather than mine.
### Tests
Three added, `TestRecordConverter` at 62 tests, 0 failures:
- nanosecond precision preserved from an ISO string and from an
`OffsetDateTime` (`.getNano() == 123456789`)
- the same for `withoutZone()` / `LocalDateTime`
- a numeric agrees with the `timestamp` path, pinning the millisecond
decision
`spotlessCheck`, `checkstyleMain` and `checkstyleTest` pass on the module.
### Deliberately not included
#15443 also mentions geometry, geography and unknown. I left those out:
- **geometry / geography** have no value representation in the generic
object model — nothing in `GenericDataUtil` or `GenericRecord`, and
`BaseParquetWriter` dispatches on Parquet primitives rather than Iceberg type
ids. Supporting them here would mean inventing a Connect-to-WKB convention with
no precedent in the generic path.
- **unknown** is documented in the spec as *"Must be optional with `null`
defaults; not stored in data files"* (spec.md:267), so the current exception
may well be correct. Whether the sink should write null instead is a behaviour
decision I did not want to make unilaterally.
Variant, the original subject of #15443, is already supported on `main` —
that issue predates it.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
