You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "vigneshc-dd (via GitHub)" <gi...@apache.org> on 2026/05/10 17:12:39 UTC
[PR] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
vigneshc-dd opened a new pull request, #16280:
URL: https://github.com/apache/iceberg/pull/16280
**Issue:** #15826 `DeleteFilter` does not nested equality delete columns when the select did not include those columns.
**Fix:**
- Use `TypeUtil.project` to preserve the struct hierarchy.
- Paths that do not provide the table schema still has the bug, this path doesn't seem to be used anywhere.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "vigneshc-dd (via GitHub)" <gi...@apache.org>.
vigneshc-dd commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3295587981
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/BaseReader.java:
##########
@@ -204,7 +204,14 @@ protected class SparkDeleteFilter extends DeleteFilter<InternalRow> {
SparkDeleteFilter(
String filePath, List<DeleteFile> deletes, DeleteCounter counter, boolean needRowPosCol) {
- super(filePath, deletes, new FieldLookup(table), expectedSchema, counter, needRowPosCol);
+ super(
+ filePath,
+ deletes,
+ table().schema(),
+ new FieldLookup(table),
Review Comment:
To fix it in `FieldLookup`,its signature should be changed to `Function<Set<Integer>, Schema>`, to handle the nesting case where we should return the path to the nested field as opposed to just the id. I could make that change, it would change the protected constructor in `DeleteFilter`. Previous approach avoided that. It did not fix the historical schema case.
I have updated `FieldLookup` now, and have removed the TODO. Added a test for historical schema case as well.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "qinghui-xu (via GitHub)" <gi...@apache.org>.
qinghui-xu commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3380535768
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
> How would an equality delete work for Arrays, and Maps?
According to the [specifications](https://iceberg.apache.org/spec/?h=identifier#identifier-field-ids), id fields cannot be arrays or maps (nor fields nested in them). So this should not be a concern.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "qinghui-xu (via GitHub)" <gi...@apache.org>.
qinghui-xu commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3402772872
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
I think we (with @pvary @vigneshc-dd) agree that we want to use `TypeUtil.project` to correctly find the (nested) columns. But there's something we may improve over your suggestion:
- method signature: in order to avoid major refactoring, we will likely keep `DeleteFilter.requiredSchema` field which is a `Schema. So it will be handy if this method returns a `Schema`
- efficiency: `TypeUtil.project` accepts a `Set` of field ids, it would be better to call it once with all the needed field ids for the sake of efficiency
- I'd also suggest to define it as instance method instead of `static`, so that it can be overridden eventually in a subclass (as apparently in spark v4.1 we implement a behavior to look up in historical schemas)
@vigneshc-dd What do you think?
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "qinghui-xu (via GitHub)" <gi...@apache.org>.
qinghui-xu commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3387769512
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
Hello @pvary
> Instead of this change, shouldn't we just introduce our own method for fieldLookup?
I was thinking you agreed with my comment on this point, so I did not address it earlier, or maybe there's some confusion. My suggestion is to introduce the method for "fieldLookup" in the `DeleteFilter` to implement the projection logic in order to correctly build the `requiredSchema` with all the nested fields. And such method can be overridden if needed in subclasses, such as in spark v4.1 implementation.
The "function" argument is a bit cumbersome for the constructor and I had a hard time to think of a clear & short naming without leading to any confusion. And such function itself is an indication of some behavior customization, so a method override in subclasses is more appropriate in this case.
> Schema.findField only returns the leaf column NestedField object, but instead we could return the whole hierarchy. Do we need any other change in this case?
I think that's what the current PR is doing?
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #16280: Data: Fix equality deletes failing for nested field identifiers.
URL: https://github.com/apache/iceberg/pull/16280
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "vigneshc-dd (via GitHub)" <gi...@apache.org>.
vigneshc-dd commented on PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#issuecomment-4634155449
@pvary any additional comments on 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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3387177585
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
Thank @qinghui-xu! Good point about the maps/arrays.
Th other question still stands:
> Instead of this change, shouldn't we just introduce our own method for fieldLookup?
> Schema.findField only returns the leaf column NestedField object, but instead we could return the whole hierarchy. Do we need any other change in this 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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#issuecomment-4953431603
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "qinghui-xu (via GitHub)" <gi...@apache.org>.
qinghui-xu commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3375152273
##########
spark/v4.1/spark/src/test/java/org/apache/iceberg/spark/data/TestHelpers.java:
##########
@@ -895,7 +896,13 @@ public static class CustomizedDeleteFilter extends DeleteFilter<InternalRow> {
protected CustomizedDeleteFilter(
boolean hasDeletes, Schema tableSchema, Schema projectedSchema) {
- super("", List.of(), tableSchema::findField, projectedSchema, new DeleteCounter(), true);
+ super(
+ "",
+ List.of(),
+ ids -> TypeUtil.project(tableSchema, ids),
Review Comment:
As you are using the "default" function, why not directly use the constructor providing it?
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
I've some thoughts on this:
- Name is a bit confusing.
- For me this should not be an argument to the constructor, instead it's better to have it as a method that can be overridden in a subclass.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#issuecomment-5017930719
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "qinghui-xu (via GitHub)" <gi...@apache.org>.
qinghui-xu commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3402772872
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
I think we (with @pvary @vigneshc-dd) agree that we want to use `TypeUtil.project` to correctly find the (nested) columns. But there's something we may improve over your suggestion:
- method signature: in order to avoid major refactoring, we will likely keep `DeleteFilter.requiredSchema` field which is a `Schema`. So it will be handy if this method returns a `Schema`
- efficiency: `TypeUtil.project` accepts a `Set` of field ids, it would be better to call it once with all the needed field ids for the sake of efficiency
- I'd also suggest to define it as instance method instead of `static`, so that it can be overridden eventually in a subclass (as apparently in spark v4.1 we implement a behavior to look up in historical schemas)
@vigneshc-dd What do you think?
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3266070833
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/source/BaseReader.java:
##########
@@ -204,7 +204,14 @@ protected class SparkDeleteFilter extends DeleteFilter<InternalRow> {
SparkDeleteFilter(
String filePath, List<DeleteFile> deletes, DeleteCounter counter, boolean needRowPosCol) {
- super(filePath, deletes, new FieldLookup(table), expectedSchema, counter, needRowPosCol);
+ super(
+ filePath,
+ deletes,
+ table().schema(),
+ new FieldLookup(table),
Review Comment:
Why can't we fix this in the `FieldLookup`?
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3379773774
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
Instead of this change, shouldn't we just introduce our own method for fieldLookup?
Schema.findField only returns the leaf column NestedField object, but instead we could return the whole hierarchy. Do we need any other change in this case?
Sidenote: How would an equality delete work for Arrays, and Maps?
--
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] Data: Fix equality deletes failing for nested field identifiers. [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16280:
URL: https://github.com/apache/iceberg/pull/16280#discussion_r3388489098
##########
data/src/main/java/org/apache/iceberg/data/DeleteFilter.java:
##########
@@ -73,13 +72,19 @@ protected DeleteFilter(
Schema expectedSchema,
DeleteCounter counter,
boolean needRowPosCol) {
- this(filePath, deletes, tableSchema::findField, expectedSchema, counter, needRowPosCol);
+ this(
+ filePath,
+ deletes,
+ ids -> TypeUtil.project(tableSchema, ids),
+ expectedSchema,
+ counter,
+ needRowPosCol);
}
protected DeleteFilter(
String filePath,
List<DeleteFile> deletes,
- Function<Integer, Types.NestedField> fieldLookup,
+ Function<Set<Integer>, Schema> missingSchemaResolver,
Review Comment:
What if we use something like this:
```
/**
* Resolves the field with the given ID within the schema. When the field is nested inside one
* or more structs, the returned field is the top-level ancestor that wraps the path down to
* the requested field so that the entire parent hierarchy is preserved. Returns {@code null}
* if the schema does not contain the field.
*/
private static Types.NestedField projectField(Schema schema, int id) {
// top-level fields can be returned directly without rebuilding any parent hierarchy
Types.NestedField topLevelField = schema.asStruct().field(id);
if (topLevelField != null) {
return topLevelField;
}
// skip projection unless the field is actually present somewhere in the struct hierarchy
if (schema.findField(id) == null) {
return null;
}
// projecting a single nested field retains the chain of parent structs that contain it
List<Types.NestedField> projected =
TypeUtil.project(schema, Collections.singleton(id)).columns();
return projected.isEmpty() ? null : projected.get(0);
}
```
--
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