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

Separate file metastore column serialization from internals #18309

Conversation

findepi
Copy link
Member

@findepi findepi commented Jul 17, 2023

Introduce separate column class representing column serialized in file metastore.

@cla-bot cla-bot bot added the cla-signed label Jul 17, 2023
@github-actions github-actions bot added tests:hive hive Hive connector labels Jul 17, 2023
@findepi
Copy link
Member Author

findepi commented Jul 17, 2023

Ideally the whole file metastore domain model could be seaprate.

@kokosing
Copy link
Member

Is it possible to generate data for file based metastore and add it to the tests to make sure it can read old data? Then when changing the internals we could have backward compability tests. WDYT?

@electrum
Copy link
Member

Why do this just for columns? If we want this, we can remove the serialization for the old class.

import static java.util.Objects.requireNonNull;

@Immutable
public class Column
Copy link
Member

Choose a reason for hiding this comment

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

We should be able to make this a record. Also, we don’t need to use explicit names for JSON serialization in new code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather not refactor this class, since we have no tests for the file metastore. I don't want to change serialization state inadvertently. From my perspective, this isn't new code.

@findepi
Copy link
Member Author

findepi commented Jul 18, 2023

Why do this just for columns?

good question
just because i want to change Column-related code (#18315)

we can remove the serialization for the old class.

good point, thanks!

@findepi findepi force-pushed the findepi/separate-file-metastore-column-serialization-from-internals-9142a9 branch from eae70c3 to abb3799 Compare July 18, 2023 15:24
@findepi findepi requested a review from electrum July 18, 2023 15:24
@findepi
Copy link
Member Author

findepi commented Jul 18, 2023

we can remove the serialization for the old class.

it appears used elsewhere (HiveInsertTableHandle["pageSinkMetadata"]->io.trino.plugin.hive.metastore.HivePageSinkMetadata["table"]->io.trino.plugin.hive.metastore.Table["dataColumns"]), will restore

Introduce separate column class representing column serialized in file
metastore.
@findepi findepi force-pushed the findepi/separate-file-metastore-column-serialization-from-internals-9142a9 branch from abb3799 to 6376631 Compare July 18, 2023 19:02
@findepi
Copy link
Member Author

findepi commented Jul 19, 2023

@electrum please take another look

@findepi
Copy link
Member Author

findepi commented Jul 20, 2023

replaced with #18343

@findepi findepi closed this Jul 20, 2023
@findepi findepi deleted the findepi/separate-file-metastore-column-serialization-from-internals-9142a9 branch July 20, 2023 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed hive Hive connector
Development

Successfully merging this pull request may close these issues.

3 participants