You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Guosmilesmile (via GitHub)" <gi...@apache.org> on 2026/03/26 15:27:43 UTC
[PR] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Guosmilesmile opened a new pull request, #15776:
URL: https://github.com/apache/iceberg/pull/15776
While working on improving the TCK for File Format, we found that in V3 tables, we support lineage in Parquet and Avro, but we haven't implemented this feature in ORC. This PR aims to add `_row_id` and `_last_updated_sequence_number` reader in ORC to support lineage.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3005029517
##########
orc/src/main/java/org/apache/iceberg/orc/ORC.java:
##########
@@ -787,11 +788,24 @@ ReadBuilder constantFieldIds(Set<Integer> newConstantFieldIds) {
public <D> CloseableIterable<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ Set<Integer> topLevelIdsToExclude =
+ Sets.difference(
+ Sets.union(constantFieldIds, MetadataColumns.metadataFieldIds()),
+ ImmutableSet.of(
+ MetadataColumns.ROW_ID.fieldId(),
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()));
Review Comment:
In the original logic, the construction of `idsToExclude` does not require metadata and is based on deleting by `fieldIds`. However, `TypeUtil.selectNot` will extract `3: id_bucket` from `_partition`, so the deletion is not thorough and will result in extra redundant information. This will cause the ORC reader to get misaligned when using the `index++` approach in the `row_id` scenario.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3154108907
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
Review Comment:
Could we call this from the old constructor?
Seems like we are deprecating a lot
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001176249
##########
orc/src/main/java/org/apache/iceberg/orc/ORC.java:
##########
@@ -787,11 +787,17 @@ ReadBuilder constantFieldIds(Set<Integer> newConstantFieldIds) {
public <D> CloseableIterable<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ Set<Integer> idsToExclude =
+ Sets.difference(
+ Sets.union(constantFieldIds, MetadataColumns.metadataFieldIds()),
+ ImmutableSet.of(
+ MetadataColumns.ROW_ID.fieldId(),
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()));
Review Comment:
This should be already in the `META_IDS`
##########
orc/src/main/java/org/apache/iceberg/orc/ORC.java:
##########
@@ -787,11 +787,17 @@ ReadBuilder constantFieldIds(Set<Integer> newConstantFieldIds) {
public <D> CloseableIterable<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ Set<Integer> idsToExclude =
+ Sets.difference(
+ Sets.union(constantFieldIds, MetadataColumns.metadataFieldIds()),
+ ImmutableSet.of(
+ MetadataColumns.ROW_ID.fieldId(),
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()));
Review Comment:
These should be already in the `META_IDS`
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001237602
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +168,51 @@ protected StructReader(
}
}
+ private int handleRowIdField(
+ int pos,
+ Types.NestedField field,
+ List<OrcValueReader<?>> readerList,
+ int readerIndex,
+ Map<Integer, ?> idToConstant) {
+ Long firstRowId = (Long) idToConstant.get(field.fieldId());
+ if (firstRowId != null) {
+ OrcValueReader<Long> fileIdReader =
+ readerIndex < readerList.size()
+ ? (OrcValueReader<Long>) readerList.get(readerIndex)
+ : null;
Review Comment:
Please help me understand why we do this
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195652099
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int dataIndex = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (field.equals(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER)) {
+ handleLastUpdatedSeqField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (idToConstant.containsKey(field.fieldId())) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(idToConstant.get(field.fieldId()));
+ } else if (field.equals(MetadataColumns.ROW_POSITION)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = new RowPositionReader();
+ } else if (field.equals(MetadataColumns.IS_DELETED)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(false);
+ } else if (fileReader != null) {
+ this.isConstantOrMetadataField[pos] = false;
+ this.fieldVectorIndex[pos] = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+ this.readers[pos] = fileReader;
+ } else if (MetadataColumns.isMetadataColumn(field.name())
+ || field.type().typeId() == Type.TypeID.UNKNOWN) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Missing ORC reader for field %s (%s)", field.name(), field.fieldId()));
+ }
+ }
+ }
+
+ private Map<Integer, Integer> buildFieldIdToVectorIndex(TypeDescription orcType) {
+ List<TypeDescription> children = orcType.getChildren();
+ Map<Integer, Integer> mapping = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i++) {
+ mapping.put(ORCSchemaUtil.fieldId(children.get(i)), i);
+ }
+
+ return mapping;
+ }
+
+ private Map<Integer, OrcValueReader<?>> readersByFieldId(
+ TypeDescription orcType, List<OrcValueReader<?>> readerList) {
+ List<TypeDescription> children = orcType.getChildren();
+ Preconditions.checkState(
+ children.size() == readerList.size(),
+ "Invalid ORC reader binding: children=%s readers=%s",
+ children.size(),
+ readerList.size());
+
+ Map<Integer, OrcValueReader<?>> readersById = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i += 1) {
+ readersById.put(ORCSchemaUtil.fieldId(children.get(i)), readerList.get(i));
+ }
+
+ return readersById;
+ }
+
+ @SuppressWarnings("unchecked")
+ private void handleRowIdField(
+ int pos,
+ Types.NestedField field,
+ OrcValueReader<?> fileReader,
+ Map<Integer, ?> idToConstant,
+ int dataIndex) {
+ Long firstRowId = (Long) idToConstant.get(field.fieldId());
+ if (firstRowId != null) {
+ OrcValueReader<Long> fileIdReader = (OrcValueReader<Long>) fileReader;
+ this.readers[pos] = new RowIdReader(firstRowId, fileIdReader);
+ this.isConstantOrMetadataField[pos] = fileIdReader == null;
+ if (fileIdReader != null) {
+ this.fieldVectorIndex[pos] = dataIndex;
+ }
+ } else {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ }
+ }
+
+ @SuppressWarnings("unchecked")
+ private void handleLastUpdatedSeqField(
+ int pos,
+ Types.NestedField field,
+ OrcValueReader<?> fileReader,
+ Map<Integer, ?> idToConstant,
+ int dataIndex) {
Review Comment:
Maybe orcIndex?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3201623918
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +180,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.orcFieldIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToOrcIndex = buildFieldIdToOrcIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int orcIndex = fieldIdToOrcIndex.getOrDefault(field.fieldId(), -1);
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, orcIndex);
+ } else if (field.equals(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER)) {
+ handleLastUpdatedSeqField(pos, field, fileReader, idToConstant, orcIndex);
+ } else if (idToConstant.containsKey(field.fieldId())) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(idToConstant.get(field.fieldId()));
+ } else if (field.equals(MetadataColumns.ROW_POSITION)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = new RowPositionReader();
+ } else if (field.equals(MetadataColumns.IS_DELETED)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(false);
+ } else if (fileReader != null) {
+ this.isConstantOrMetadataField[pos] = false;
+ this.orcFieldIndex[pos] = fieldIdToOrcIndex.getOrDefault(field.fieldId(), -1);
+ this.readers[pos] = fileReader;
+ } else if (MetadataColumns.isMetadataColumn(field.name())
+ || field.type().typeId() == Type.TypeID.UNKNOWN) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Missing ORC reader for field %s (%s)", field.name(), field.fieldId()));
+ }
+ }
+ }
+
+ private Map<Integer, Integer> buildFieldIdToOrcIndex(TypeDescription orcType) {
+ List<TypeDescription> children = orcType.getChildren();
+ Map<Integer, Integer> mapping = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i++) {
+ mapping.put(ORCSchemaUtil.fieldId(children.get(i)), i);
Review Comment:
What happens with old ORC files here?
When the Iceberg `ICEBERG_ID_ATTRIBUTE` is not yet set in the ORC column metadata?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195650450
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int dataIndex = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (field.equals(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER)) {
+ handleLastUpdatedSeqField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (idToConstant.containsKey(field.fieldId())) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(idToConstant.get(field.fieldId()));
+ } else if (field.equals(MetadataColumns.ROW_POSITION)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = new RowPositionReader();
+ } else if (field.equals(MetadataColumns.IS_DELETED)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(false);
+ } else if (fileReader != null) {
+ this.isConstantOrMetadataField[pos] = false;
+ this.fieldVectorIndex[pos] = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+ this.readers[pos] = fileReader;
+ } else if (MetadataColumns.isMetadataColumn(field.name())
+ || field.type().typeId() == Type.TypeID.UNKNOWN) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Missing ORC reader for field %s (%s)", field.name(), field.fieldId()));
+ }
+ }
+ }
+
+ private Map<Integer, Integer> buildFieldIdToVectorIndex(TypeDescription orcType) {
+ List<TypeDescription> children = orcType.getChildren();
+ Map<Integer, Integer> mapping = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i++) {
+ mapping.put(ORCSchemaUtil.fieldId(children.get(i)), i);
+ }
+
+ return mapping;
+ }
+
+ private Map<Integer, OrcValueReader<?>> readersByFieldId(
+ TypeDescription orcType, List<OrcValueReader<?>> readerList) {
+ List<TypeDescription> children = orcType.getChildren();
+ Preconditions.checkState(
+ children.size() == readerList.size(),
+ "Invalid ORC reader binding: children=%s readers=%s",
+ children.size(),
+ readerList.size());
+
+ Map<Integer, OrcValueReader<?>> readersById = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i += 1) {
+ readersById.put(ORCSchemaUtil.fieldId(children.get(i)), readerList.get(i));
+ }
+
+ return readersById;
+ }
+
+ @SuppressWarnings("unchecked")
+ private void handleRowIdField(
+ int pos,
+ Types.NestedField field,
+ OrcValueReader<?> fileReader,
+ Map<Integer, ?> idToConstant,
+ int dataIndex) {
Review Comment:
Maybe orcIndex?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3005029517
##########
orc/src/main/java/org/apache/iceberg/orc/ORC.java:
##########
@@ -787,11 +788,24 @@ ReadBuilder constantFieldIds(Set<Integer> newConstantFieldIds) {
public <D> CloseableIterable<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ Set<Integer> topLevelIdsToExclude =
+ Sets.difference(
+ Sets.union(constantFieldIds, MetadataColumns.metadataFieldIds()),
+ ImmutableSet.of(
+ MetadataColumns.ROW_ID.fieldId(),
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()));
Review Comment:
In the original logic, the construction of `idsToExclude` does not require metadata and is based on deleting by `fieldIds`. However, `TypeUtil.selectNot` will extract `3: id_bucket` from `_partition`, so the deletion is not thorough and will result in extra redundant information. This will cause the ORC reader to get misaligned when using the `index++` approach in the `row_id` scenario.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001203389
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -141,9 +141,14 @@ protected StructReader(
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
- for (int pos = 0, readerIndex = 0; pos < fields.size(); pos += 1) {
+ int readerIndex = 0;
Review Comment:
Oh.. it is just the reader index!
Good
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001237856
##########
orc/src/main/java/org/apache/iceberg/orc/ORC.java:
##########
@@ -787,11 +787,17 @@ ReadBuilder constantFieldIds(Set<Integer> newConstantFieldIds) {
public <D> CloseableIterable<D> build() {
Preconditions.checkNotNull(schema, "Schema is required");
+ Set<Integer> idsToExclude =
+ Sets.difference(
+ Sets.union(constantFieldIds, MetadataColumns.metadataFieldIds()),
+ ImmutableSet.of(
+ MetadataColumns.ROW_ID.fieldId(),
+ MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER.fieldId()));
Review Comment:
Yes, META_IDS contains the row ID and last update sequence. The original code would delete all metadata-related fields, but in the lineage scenario, `_row_id` exists in the datafile and should not be removed. Therefore, we need to use `difference` to remove `ROW_ID` and `LAST_UPDATED_SEQUENCE_NUMBER` here.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195642183
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -135,12 +138,24 @@ public byte[] nonNullRead(ColumnVector vector, int row) {
public abstract static class StructReader<T> implements OrcValueReader<T> {
private final OrcValueReader<?>[] readers;
private final boolean[] isConstantOrMetadataField;
-
+ private final int[] fieldVectorIndex;
+
+ /**
+ * @param readers readers for each field
+ * @param struct struct type
+ * @param idToConstant constant values by field id
+ * @deprecated Use {@link #StructReader(TypeDescription, List, Types.StructType, Map)} instead.
+ * This constructor uses position-based binding which may cause field misalignment in MOR
+ * scenarios. This doesn't work lineage scenarios.
+ */
+ @Deprecated
protected StructReader(
List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = null;
Review Comment:
It is hard to understand what this attribute stores.
Maybe call it `orcFieldIndex` and add some comment?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195646600
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
Review Comment:
Rename `fieldIdToDataIndex` to highlight that this is for the orc position (maybe to `fieldIdToOrcIndex`)?
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
Review Comment:
Rename `fieldIdToDataIndex` to highlight that this is for the orc schema position (maybe to `fieldIdToOrcIndex`)?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3196426694
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -135,12 +138,24 @@ public byte[] nonNullRead(ColumnVector vector, int row) {
public abstract static class StructReader<T> implements OrcValueReader<T> {
private final OrcValueReader<?>[] readers;
private final boolean[] isConstantOrMetadataField;
-
+ private final int[] fieldVectorIndex;
+
+ /**
+ * @param readers readers for each field
+ * @param struct struct type
+ * @param idToConstant constant values by field id
+ * @deprecated Use {@link #StructReader(TypeDescription, List, Types.StructType, Map)} instead.
+ * This constructor uses position-based binding which may cause field misalignment in MOR
+ * scenarios. This doesn't work lineage scenarios.
+ */
+ @Deprecated
protected StructReader(
List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = null;
Review Comment:
Thanks for the new name suggestion. I've also added a comment for 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: 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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3167975164
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
Review Comment:
Could this change cause issues elsewhere?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3180896143
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
Review Comment:
No, it won't. With the current changes, if the old constructor is used, the original behavior is preserved, and the new constructor has the new behavior. If that's the case, wouldn't it be better to not add Deprecated, but instead write proper Javadoc and let the caller choose based on their own scenario?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3154092355
##########
orc/src/main/java/org/apache/iceberg/data/orc/GenericOrcReaders.java:
##########
@@ -56,11 +57,25 @@ public class GenericOrcReaders {
private GenericOrcReaders() {}
+ /**
+ * @deprecated Use {@link #struct(TypeDescription, List, Types.StructType, Map)} instead. This
+ * method uses position-based binding which may cause field misalignment in MOR and lineage
+ * scenarios.
+ */
Review Comment:
Is this really an issue on MOR case?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001187949
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -141,9 +141,14 @@ protected StructReader(
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
- for (int pos = 0, readerIndex = 0; pos < fields.size(); pos += 1) {
+ int readerIndex = 0;
Review Comment:
We have `RowPositionReader` which handles that
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3154625801
##########
orc/src/main/java/org/apache/iceberg/data/orc/GenericOrcReaders.java:
##########
@@ -56,11 +57,25 @@ public class GenericOrcReaders {
private GenericOrcReaders() {}
+ /**
+ * @deprecated Use {@link #struct(TypeDescription, List, Types.StructType, Map)} instead. This
+ * method uses position-based binding which may cause field misalignment in MOR and lineage
+ * scenarios.
+ */
Review Comment:
Initially, I was reading by position based on the original approach, but it failed in Spark's TestMergeOnReadWithLineage while TestCopyOnWriteWithLineage passed correctly. That's why the MOR distinction came 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: 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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3154667346
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
Review Comment:
The new constructor is not just an overload; it changes the binding model from positional to field-id-based, and that requires `TypeDescription`. So the old constructor cannot meaningfully forward to 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: 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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3180932578
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToVectorIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, fieldIdToVectorIndex);
Review Comment:
Rename fieldIdToVectorIndex to fieldIdToDataIndex, then pushdown dataIndex into method.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001182541
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -141,9 +141,14 @@ protected StructReader(
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
- for (int pos = 0, readerIndex = 0; pos < fields.size(); pos += 1) {
+ int readerIndex = 0;
Review Comment:
How do we handle rows when the filter pushdown removes them?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#issuecomment-4406905574
Merged to main.
Thanks @Guosmilesmile for the 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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#issuecomment-4389482563
Rebase main. Apologies, I accidentally added some unrelated labels in my initial operation. They have been corrected now.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195655256
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int dataIndex = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (field.equals(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER)) {
+ handleLastUpdatedSeqField(pos, field, fileReader, idToConstant, dataIndex);
+ } else if (idToConstant.containsKey(field.fieldId())) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(idToConstant.get(field.fieldId()));
+ } else if (field.equals(MetadataColumns.ROW_POSITION)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = new RowPositionReader();
+ } else if (field.equals(MetadataColumns.IS_DELETED)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(false);
+ } else if (fileReader != null) {
+ this.isConstantOrMetadataField[pos] = false;
+ this.fieldVectorIndex[pos] = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
+ this.readers[pos] = fileReader;
+ } else if (MetadataColumns.isMetadataColumn(field.name())
+ || field.type().typeId() == Type.TypeID.UNKNOWN) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Missing ORC reader for field %s (%s)", field.name(), field.fieldId()));
+ }
+ }
+ }
+
+ private Map<Integer, Integer> buildFieldIdToVectorIndex(TypeDescription orcType) {
Review Comment:
Maybe `buildFieldIdToOrcIndex`?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195663443
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -135,12 +138,24 @@ public byte[] nonNullRead(ColumnVector vector, int row) {
public abstract static class StructReader<T> implements OrcValueReader<T> {
private final OrcValueReader<?>[] readers;
private final boolean[] isConstantOrMetadataField;
-
+ private final int[] fieldVectorIndex;
+
+ /**
+ * @param readers readers for each field
+ * @param struct struct type
+ * @param idToConstant constant values by field id
+ * @deprecated Use {@link #StructReader(TypeDescription, List, Types.StructType, Map)} instead.
+ * This constructor uses position-based binding which may cause field misalignment in MOR
+ * scenarios. This doesn't work lineage scenarios.
+ */
+ @Deprecated
protected StructReader(
List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
List<Types.NestedField> fields = struct.fields();
this.readers = new OrcValueReader[fields.size()];
this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = null;
Review Comment:
Maybe add a short comment to describe what is stored here
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3195648982
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToDataIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int dataIndex = fieldIdToDataIndex.getOrDefault(field.fieldId(), -1);
Review Comment:
Maybe orcIndex?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3203015127
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +180,122 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.orcFieldIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToOrcIndex = buildFieldIdToOrcIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+ int orcIndex = fieldIdToOrcIndex.getOrDefault(field.fieldId(), -1);
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, orcIndex);
+ } else if (field.equals(MetadataColumns.LAST_UPDATED_SEQUENCE_NUMBER)) {
+ handleLastUpdatedSeqField(pos, field, fileReader, idToConstant, orcIndex);
+ } else if (idToConstant.containsKey(field.fieldId())) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(idToConstant.get(field.fieldId()));
+ } else if (field.equals(MetadataColumns.ROW_POSITION)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = new RowPositionReader();
+ } else if (field.equals(MetadataColumns.IS_DELETED)) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(false);
+ } else if (fileReader != null) {
+ this.isConstantOrMetadataField[pos] = false;
+ this.orcFieldIndex[pos] = fieldIdToOrcIndex.getOrDefault(field.fieldId(), -1);
+ this.readers[pos] = fileReader;
+ } else if (MetadataColumns.isMetadataColumn(field.name())
+ || field.type().typeId() == Type.TypeID.UNKNOWN) {
+ this.isConstantOrMetadataField[pos] = true;
+ this.readers[pos] = constants(null);
+ } else {
+ throw new IllegalArgumentException(
+ String.format("Missing ORC reader for field %s (%s)", field.name(), field.fieldId()));
+ }
+ }
+ }
+
+ private Map<Integer, Integer> buildFieldIdToOrcIndex(TypeDescription orcType) {
+ List<TypeDescription> children = orcType.getChildren();
+ Map<Integer, Integer> mapping = Maps.newHashMap();
+ for (int i = 0; i < children.size(); i++) {
+ mapping.put(ORCSchemaUtil.fieldId(children.get(i)), i);
Review Comment:
Old ORC files are handled before this code path reaches `StructReader`. In OrcIterable, if `ORCSchemaUtil.hasIds(fileSchema)` is false, `OrcIterable` applies a name mapping to the file schema
https://github.com/apache/iceberg/blob/f767dad20e9a47495ce37f1acf6acdffbf63664d/orc/src/main/java/org/apache/iceberg/orc/OrcIterable.java#L87-L96
`applyNameMapping` sets `ICEBERG_ID_ATTRIBUTE` on the in-memory TypeDescription, so `StructReader` receives a projected ORC schema with field IDs.
So StructReader receives the projected readOrcSchema with field IDs, not the raw id-less file schema.
This assumes the reader is constructed through the normal OrcIterable path; directly passing a raw
id-less ORC schema to StructReader would still be invalid.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #15776:
URL: https://github.com/apache/iceberg/pull/15776
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3001479977
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +168,51 @@ protected StructReader(
}
}
+ private int handleRowIdField(
+ int pos,
+ Types.NestedField field,
+ List<OrcValueReader<?>> readerList,
+ int readerIndex,
+ Map<Integer, ?> idToConstant) {
+ Long firstRowId = (Long) idToConstant.get(field.fieldId());
+ if (firstRowId != null) {
+ OrcValueReader<Long> fileIdReader =
+ readerIndex < readerList.size()
+ ? (OrcValueReader<Long>) readerList.get(readerIndex)
+ : null;
Review Comment:
I understand that `readerList` represents physical columns, while `ROW_ID`/`LAST_UPDATED_SEQUENCE_NUMBER` may only exist in the logical projection. Although in my testing the counts are consistent, I cannot guarantee that there are no other scenarios where the projection and physical fields are inconsistent. So I added `fileIdReader == null` to fall back to the fallback path, which has a bit of a defensive programming flavor.
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#issuecomment-4143459046
Add ut for spark , this pr don't implemented lineage in spark vector read in ORC, it will support it in the follow 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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3154098355
##########
orc/src/main/java/org/apache/iceberg/data/orc/GenericOrcReaders.java:
##########
@@ -56,11 +57,25 @@ public class GenericOrcReaders {
private GenericOrcReaders() {}
+ /**
+ * @deprecated Use {@link #struct(TypeDescription, List, Types.StructType, Map)} instead. This
+ * method uses position-based binding which may cause field misalignment in MOR and lineage
+ * scenarios.
+ */
+ @Deprecated
public static OrcValueReader<Record> struct(
List<OrcValueReader<?>> readers, Types.StructType struct, Map<Integer, ?> idToConstant) {
return new StructReader(readers, struct, idToConstant);
}
+ public static OrcValueReader<Record> struct(
+ TypeDescription record,
Review Comment:
Instead of `record` could we use `orcType` or something similar?
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#issuecomment-4332542632
Rebase main
--
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] ORC: Add _row_id and _last_updated_sequence_number raeder in Orc to support lineage [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #15776:
URL: https://github.com/apache/iceberg/pull/15776#discussion_r3167987041
##########
orc/src/main/java/org/apache/iceberg/orc/OrcValueReaders.java:
##########
@@ -163,6 +177,121 @@ protected StructReader(
}
}
+ protected StructReader(
+ TypeDescription orcType,
+ List<OrcValueReader<?>> readers,
+ Types.StructType struct,
+ Map<Integer, ?> idToConstant) {
+ List<Types.NestedField> fields = struct.fields();
+ this.readers = new OrcValueReader[fields.size()];
+ this.isConstantOrMetadataField = new boolean[fields.size()];
+ this.fieldVectorIndex = new int[fields.size()];
+
+ Map<Integer, OrcValueReader<?>> readersById = readersByFieldId(orcType, readers);
+ Map<Integer, Integer> fieldIdToVectorIndex = buildFieldIdToVectorIndex(orcType);
+
+ for (int pos = 0; pos < fields.size(); pos += 1) {
+ Types.NestedField field = fields.get(pos);
+ OrcValueReader<?> fileReader = readersById.get(field.fieldId());
+
+ if (field.equals(MetadataColumns.ROW_ID)) {
+ handleRowIdField(pos, field, fileReader, idToConstant, fieldIdToVectorIndex);
Review Comment:
Could we just push down the value instead of the full index?
Maybe fieldIdToDataIndex if needed
--
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