Skip to content
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.

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

Parquet: update PruneColumns to inherit from TypeWithSchemaVisitor to have Iceberg type #11179

Merged

Conversation

aihuaxu
Copy link
Contributor

@aihuaxu aihuaxu commented Sep 20, 2024

Currently PruneColumns inherits from ParquetTypeVisitor which will only pass Parquet types. Later when we add the support for Variant type, we need to know the column is a Variant type during column pruning. Before logical Variant type is added to Parquet, we can change to inherit from TypeWithSchemaVisitor to have the Iceberg type. #11178

Existing tests in TestPruneColumns should provide the test coverage so no tests are added.

@aihuaxu
Copy link
Contributor Author

aihuaxu commented Sep 20, 2024

cc @rdblue and @RussellSpitzer

@@ -40,7 +43,7 @@ class PruneColumns extends ParquetTypeVisitor<Type> {
}

@Override
public Type message(MessageType message, List<Type> fields) {
public Type message(StructType iStruct, MessageType message, List<Type> fields) {
Copy link
Member

@RussellSpitzer RussellSpitzer Sep 25, 2024

Choose a reason for hiding this comment

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

We generally avoid names like iStruct, I would suggest something like tableStruct

Copy link
Member

Choose a reason for hiding this comment

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

Checking other versions, we generally use expected for this struct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. We do use expected elsewhere. Updated.

@RussellSpitzer
Copy link
Member

Thanks @aihuaxu , This should make our Variant work a lot easier (also potentially geometric work!)

@RussellSpitzer RussellSpitzer merged commit 2d9c344 into apache:main Sep 26, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants