vbhanuchander-lang commented on issue #15443:
URL: https://github.com/apache/iceberg/issues/15443#issuecomment-5251974383
I went through this against current `main` and the picture has changed since
February. Summarising so the issue reflects what is actually left.
**Variant is supported now.** `RecordConverter` has both the dispatch and
the implementation:
```java
// RecordConverter.java:165
case VARIANT:
return convertVariantValue(value);
// RecordConverter.java:578
protected Variant convertVariantValue(Object value) {
if (value instanceof Variant variant) { ...
```
So the original report is resolved. (I could not pin which release it landed
in — my clone is shallow, so I am not claiming a version.)
**The second half of the report still holds, but for three types, and they
are not equivalent.** Diffing `Type.TypeID` against the switch, the unhandled
ones are `TIMESTAMP_NANO`, `GEOMETRY`, `GEOGRAPHY` and `UNKNOWN`, each falling
through to:
```java
throw new UnsupportedOperationException("Unsupported type: " +
type.typeId());
```
Taking them separately, because I do not think they should be fixed together:
**`TIMESTAMP_NANO` — a real gap, and implementable here.** The generic
object model already handles it, so the sink is 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);
}
```
This looks like a straightforward sibling of the existing `case TIMESTAMP`
path.
**`UNKNOWN` — the exception may be correct.** The spec says this type is not
written at all:
> `unknown` — Default / null column type used when a more specific type is
not known — **Must be optional with `null` defaults; not stored in data files**
(spec.md:267)
> All columns of `unknown`, `variant`, `geometry`, and `geography` types
must default to null. Non-null values for `initial-default` or `write-default`
are invalid. (spec.md:335)
So the open question is whether the sink should write null rather than
throw, which is a behaviour decision rather than a missing conversion. I would
not change it without a maintainer's view.
**`GEOMETRY` / `GEOGRAPHY` — blocked below this layer, not in the
converter.** There is no geometry value representation in the generic object
model: nothing in `GenericDataUtil` or `GenericRecord`, and `BaseParquetWriter`
dispatches on the *Parquet* primitive name plus logical annotations rather than
on Iceberg type ids. Adding a `case GEOMETRY` to `RecordConverter` would mean
inventing the Connect-to-WKB convention here, with no precedent elsewhere in
the generic path and no test coverage to match. That seems like it belongs in a
geospatial-support discussion for the generic writers, not in this sink.
---
@alexkot1394 you attached a working `RecordConverter.java` back in March —
thank you, and apologies if you are already mid-flight on this. Are you
planning a PR? I do not want to duplicate you.
Otherwise, for the maintainers: I am happy to implement `TIMESTAMP_NANO` on
its own, following the existing `TIMESTAMP` pattern with tests. Two things I
would want confirmed before writing it, since both change the scope:
1. Is the Kafka Connect converter the right place, or would you rather
nanosecond timestamps were handled generically for all `Record`-based writers?
2. Should `UNKNOWN` write null instead of throwing, or is the current
exception intended?
Retitling this to something like "Kafka Connect: support timestamp_ns;
geometry/geography blocked on generic writers" would also make the remaining
scope clearer, if that is welcome.
--
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]
