Skip to content

[LI] Bug fix: Remove ids from fileSchema before feeding it into apply… #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rzhang10 merged 3 commits into from
Feb 2, 2023
Merged

[LI] Bug fix: Remove ids from fileSchema before feeding it into apply… #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0dc28d4
[LI] Bug fix: Remove ids from fileSchema before feeding it into apply…
rzhang10 Jan 26, 2023
74b5ba4
Add union handling in ORC RemoveIds
rzhang10 Jan 28, 2023
75b7e3a
Fix checkstyle
rzhang10 Jan 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion orc/src/main/java/org/apache/iceberg/orc/OrcIterable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,9 @@ public CloseableIterator<T> iterator() {
if (nameMapping == null) {
nameMapping = MappingUtil.create(schema);
}
fileSchemaWithIds = ORCSchemaUtil.applyNameMapping(fileSchema, nameMapping);
// Since in the above branch, if the ignoreFileFieldIds is true, fileSchema can still have ids,
// here we should first sanitize it by removing the existing ids.
fileSchemaWithIds = ORCSchemaUtil.applyNameMapping(ORCSchemaUtil.removeIds(fileSchema), nameMapping);
}
readOrcSchema = ORCSchemaUtil.buildOrcProjection(schema, fileSchemaWithIds);
// If the projected ORC schema is an empty struct, it means we are only projecting columns
Expand Down
7 changes: 7 additions & 0 deletions orc/src/main/java/org/apache/iceberg/orc/RemoveIds.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,13 @@ public TypeDescription map(TypeDescription map, TypeDescription key, TypeDescrip
return TypeDescription.createMap(key, value);
}

@Override
public TypeDescription union(TypeDescription union, List<TypeDescription> options) {
Copy link

@yiqiangin yiqiangin Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify a little more on why this function is needed for this PR?

Copy link
Member Author

@rzhang10 rzhang10 Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are leveraging the RemoveIds visitor to remove ids from a ORC schema, the issue is in LI-Iceberg we added union type support whereas the vanilla RemoveIds doesn't have the union case (because vanilla iceberg doesn't have union type), so we just need to add this override implementation to reconstruct the union schema case.

TypeDescription ret = TypeDescription.createUnion();
options.forEach(ret::addUnionChild);
return ret;
}

@Override
public TypeDescription primitive(TypeDescription primitive) {
return removeIcebergAttributes(primitive.clone());
Expand Down