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.

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

Jump to bottom

Write correct Iceberg types to Glue metastore #18299

Closed
wants to merge 5 commits into from
Closed

Write correct Iceberg types to Glue metastore #18299

wants to merge 5 commits into from

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 14, 2023

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 14, 2023
@findepi findepi marked this pull request as draft July 14, 2023 17:44
@findepi findepi mentioned this pull request Jul 14, 2023
Merged
@findepi
Copy link
Member Author

findepi commented Jul 14, 2023

Note that Iceberg own GlueCatalog has similar problem:
https://github.com/apache/iceberg/blob/370c135144ce2d1b00e9938e267548e78ee6edaf/aws/src/main/java/org/apache/iceberg/aws/glue/IcebergToGlueConverter.java#L263-L268

Relevant discussion on iceberg slack: https://apache-iceberg.slack.com/archives/C03LG1D563F/p1689351883036489

alexjo2144
alexjo2144 reviewed Jul 14, 2023
View reviewed changes
Copy link
Member

@alexjo2144 alexjo2144 left a comment

Choose a reason for hiding this comment

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

I didn't think we wrote any column information for Iceberg tables to Glue, am I wrong about that?

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/Column.java
import static com.google.common.collect.ImmutableList.toImmutableList;
import static java.util.Objects.requireNonNull;

@Immutable
Copy link
Member

Choose a reason for hiding this comment

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

Are we annotating immutable classes now? That's new to me

Copy link
Member Author

Choose a reason for hiding this comment

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

We sometimes do. I think this annotation is generally desirable. This class is a copy of other one, and i just copied annotation anyway.

@findepi
Copy link
Member Author

findepi commented Jul 14, 2023

I didn't think we wrote any column information for Iceberg tables to Glue, am I wrong about that?

you're right. I didn't finish the work.

@findepi findepi force-pushed the findepi/iceberg-is-columns branch 2 times, most recently from 8f9899e to a4e8a5e Compare July 14, 2023 19:48
@findepi findepi mentioned this pull request Jul 14, 2023
Merged
@github-actions github-actions bot added tests:hive iceberg Iceberg connector hive Hive connector labels Jul 15, 2023
@findepi findepi mentioned this pull request Jul 17, 2023
Merged
findepi added 5 commits July 17, 2023 17:51
@findepi
Set column types in Iceberg register_table
b46a0e8 
HiveCatalog sets data column types in metastore for an Iceberg table.
This adds same behavior for register_table flow.
@findepi
Separate file metastore column serialization from internals
1aa8498 
Introduce separate column class representing column serialized in file
metastore.
@findepi
Change internal metastore column type to String
c875fff 
Previously we were modelling metastore types with `HiveType`, which was
appropriate for Hive connector only. This changes internal model to use
`String`, allowing connectors to store all needed types in the
metastore.
@findepi
Correct Iceberg types written to metastore
7295970
@findepi
Write Iceberg column types to Glue
1d74afa
@findepi findepi force-pushed the findepi/iceberg-is-columns branch from a4e8a5e to 1d74afa Compare July 17, 2023 15:51
@findepi
Copy link
Member Author

findepi commented Jul 21, 2023

Superseded by #18315

@findepi findepi closed this Jul 21, 2023
@findepi findepi deleted the findepi/iceberg-is-columns branch July 21, 2023 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexjo2144 alexjo2144 alexjo2144 left review comments

Assignees
No one assigned
Labels
cla-signed hive Hive connector iceberg Iceberg connector
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @alexjo2144