laskoviymishka commented on code in PR #16654:
URL: https://github.com/apache/iceberg/pull/16654#discussion_r3737182757
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/RecordConverter.java:
##########
@@ -95,11 +95,19 @@ class RecordConverter {
private final NameMapping nameMapping;
private final IcebergSinkConfig config;
private final Map<Integer, Map<String, NestedField>> structNameMap =
Maps.newHashMap();
+ // Parquet stores UUIDs as a 16-byte fixed; other formats keep the UUID
logical type. The write
+ // file format is fixed for the converter's lifetime, so resolve this once
instead of per value.
+ private final boolean writeUuidAsBytes;
RecordConverter(Table table, IcebergSinkConfig config) {
this.tableSchema = table.schema();
this.nameMapping = createNameMapping(table);
this.config = config;
+ this.writeUuidAsBytes =
Review Comment:
Reading `config.writeProps()` here means any `RecordConverter` built from a
bare `IcebergSinkConfig` mock now NPEs at construction rather than lazily
inside `convertUUID`. That's arguably the healthier failure mode, and you've
covered it with the `@BeforeEach` default — just worth a quick check that no
other test in the module constructs a converter with an unstubbed config.
It also freezes the value at construction, so a future test that constructs
first and re-stubs `writeProps()` afterward would silently skip the Parquet
branch. Not a problem today, but the ordering constraint is now implicit — a
one-line comment on the field would save someone a confusing debug session.
wdyt?
##########
kafka-connect/kafka-connect/src/main/java/org/apache/iceberg/connect/data/RecordConverter.java:
##########
@@ -95,11 +95,19 @@ class RecordConverter {
private final NameMapping nameMapping;
private final IcebergSinkConfig config;
private final Map<Integer, Map<String, NestedField>> structNameMap =
Maps.newHashMap();
+ // Parquet stores UUIDs as a 16-byte fixed; other formats keep the UUID
logical type. The write
+ // file format is fixed for the converter's lifetime, so resolve this once
instead of per value.
+ private final boolean writeUuidAsBytes;
RecordConverter(Table table, IcebergSinkConfig config) {
this.tableSchema = table.schema();
this.nameMapping = createNameMapping(table);
this.config = config;
+ this.writeUuidAsBytes =
+ FileFormat.PARQUET
+ .name()
+ .toLowerCase(Locale.ROOT)
+
.equals(config.writeProps().get(TableProperties.DEFAULT_FILE_FORMAT));
Review Comment:
Pre-existing, but since it moved into new code: the comparison is
case-sensitive on the property value, so a user who sets
`write.format.default=PARQUET` (or `Parquet`) gets `writeUuidAsBytes=false` and
the UUID stored as an object instead of bytes.
`FileFormat.PARQUET.name().equalsIgnoreCase(...)` would harden it. Optional,
and fine to leave for a follow-up.
--
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]
