You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rdblue (via GitHub)" <gi...@apache.org> on 2025/03/19 20:24:34 UTC
[PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
rdblue opened a new pull request, #12581:
URL: https://github.com/apache/iceberg/pull/12581
This fixes ORC and Parquet writers with unknown columns.
Previously, writers assumed that the Iceberg schema of the incoming data and the outgoing Parquet schema matched. However, unknown is not represented in file schemas, which led to an index mismatch. The value for an unknown column was being used by index and passed to the next writer. In some cases, this is not caught because the data is valid when the fields did not align (for example if there is only one more field and it is optional).
The fix is to pass the Iceberg schema into the write builder and account for columns that are present in data records but are not passed to a writer.
This updates all paths that called Parquet's `GenericParquetWriter` or `InternalWriter` to pass the data schema. This was not necessary in ORC because the write builder already passes the data schema.
Avro does not need to be fixed because unknown is converted to a NULL schema and a null writer is used.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#issuecomment-2741736285
Thanks for the reviews, @pvary and @danielcweeks!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2004240939
##########
core/src/test/java/org/apache/iceberg/data/DataTest.java:
##########
@@ -148,7 +148,11 @@ public void testTypeSchema(Type type) throws IOException {
.as("variant is not yet implemented")
.isTrue();
- writeAndValidate(new Schema(required(1, "id", LongType.get()), optional(2, "test_type", type)));
+ writeAndValidate(
+ new Schema(
+ required(1, "id", LongType.get()),
+ optional(2, "test_type", type),
+ required(3, "trailing_data", Types.StringType.get())));
Review Comment:
Without correct alignment, this test will fail for unknown when the `null` value for field 2 is passed to the writer for field 3.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #12581:
URL: https://github.com/apache/iceberg/pull/12581
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2006421739
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalWriter.java:
##########
@@ -40,11 +40,15 @@ public class InternalWriter<T extends StructLike> extends BaseParquetWriter<T> {
private InternalWriter() {}
+ /**
+ * @deprecated will be removed in 1.10.0; use {@link #createWriter(Schema, MessageType)} instead.
+ */
+ @Deprecated
public static <T extends StructLike> ParquetValueWriter<T> create(MessageType type) {
return create((Types.StructType) null, type);
}
- public static <T extends StructLike> ParquetValueWriter<T> create(
+ public static <T extends StructLike> ParquetValueWriter<T> createWriter(
Review Comment:
I don't love this because we now have a confusing combination of `create`, `createWriter` and `buildWriter` floating around, but I also couldn't find a good alternative.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2005681428
##########
orc/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java:
##########
@@ -156,8 +156,8 @@ public Stream<FieldMetrics<?>> metrics() {
private static class RecordWriter extends GenericOrcWriters.StructWriter<Record> {
- RecordWriter(List<OrcValueWriter<?>> writers) {
- super(writers);
+ RecordWriter(Types.StructType struct, List<OrcValueWriter<?>> writers) {
+ super(struct, writers);
Review Comment:
Do we need a similar change in Flink/Spark ORC writers?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2005949897
##########
orc/src/main/java/org/apache/iceberg/data/orc/GenericOrcWriter.java:
##########
@@ -156,8 +156,8 @@ public Stream<FieldMetrics<?>> metrics() {
private static class RecordWriter extends GenericOrcWriters.StructWriter<Record> {
- RecordWriter(List<OrcValueWriter<?>> writers) {
- super(writers);
+ RecordWriter(Types.StructType struct, List<OrcValueWriter<?>> writers) {
+ super(struct, writers);
Review Comment:
We will. Right now, Spark writers and Flink ORC writers don't support the new types so they don't need to be updated in this PR.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2006502201
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalWriter.java:
##########
@@ -40,11 +40,15 @@ public class InternalWriter<T extends StructLike> extends BaseParquetWriter<T> {
private InternalWriter() {}
+ /**
+ * @deprecated will be removed in 1.10.0; use {@link #createWriter(Schema, MessageType)} instead.
+ */
+ @Deprecated
public static <T extends StructLike> ParquetValueWriter<T> create(MessageType type) {
return create((Types.StructType) null, type);
}
- public static <T extends StructLike> ParquetValueWriter<T> create(
+ public static <T extends StructLike> ParquetValueWriter<T> createWriter(
Review Comment:
I think we should just be more aggressive about fixing these. We should deprecate the old ones and support better names.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2004256920
##########
parquet/src/main/java/org/apache/iceberg/parquet/ParquetValueWriters.java:
##########
@@ -648,7 +656,7 @@ protected StructWriter(List<ParquetValueWriter<?>> writers) {
@Override
public void write(int repetitionLevel, S value) {
for (int i = 0; i < writers.length; i += 1) {
- Object fieldValue = get(value, i);
+ Object fieldValue = get(value, fieldIndexes[i]);
Review Comment:
The fix is to map from writer index to field index. The index is created below by skipping unknown fields in the data struct.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#discussion_r2004250123
##########
parquet/src/main/java/org/apache/iceberg/data/parquet/InternalWriter.java:
##########
@@ -40,11 +40,15 @@ public class InternalWriter<T extends StructLike> extends BaseParquetWriter<T> {
private InternalWriter() {}
+ /**
+ * @deprecated will be removed in 1.10.0; use {@link #createWriter(Schema, MessageType)} instead.
+ */
+ @Deprecated
public static <T extends StructLike> ParquetValueWriter<T> create(MessageType type) {
return create((Types.StructType) null, type);
}
- public static <T extends StructLike> ParquetValueWriter<T> create(
+ public static <T extends StructLike> ParquetValueWriter<T> createWriter(
Review Comment:
This was needed because `InternalWriter::create` could refer to either `create(MessageType)` or `create(Schema, MessageType)`. Adding the ability to pass `BiFunction<Schema, MessageType, ParquetValueWriter<?>>` to `Parquet.createWriterFunc` caused compile failures because the calls were ambiguous.
To solve the problem, I renamed this method so it is unambiguous. This has not been in a release so it is safe. The `create(MessageType)` method has been in a release so it is deprecated for removal.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core, Parquet, ORC: Fix missing data when writing unknown [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #12581:
URL: https://github.com/apache/iceberg/pull/12581#issuecomment-2738144462
You might want to run `gradle revapi` because I think there are a few issues.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org