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

Refactor/object store in table #149

Merged
merged 2 commits into from
Mar 25, 2025
Merged

Conversation

JanKaul
Copy link
Owner

@JanKaul JanKaul commented Mar 24, 2025

Closes #117

This PR creates an object_store field for Tabular structs to have a reference to the underlying ObjectStore. Before this was part of the catalog trait. This simplifies setting up the object_store with credentials for individual tables.

Copy link
Contributor

@gruuya gruuya left a comment

Choose a reason for hiding this comment

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

Thank you, overall I think this is improves the flexibility of Catalog/Tabular objects!

That said, I still suspect that Catalogs should not have the object_store_builder field, at least not the RestCatalog (or it should be made into a trait). This is because, as clients of the catalog we really don't need to know the connection options for the object_store ahead of time (or at all)—all the adequate options should be returned within the load/create tabular responses.

In other words, we only need to know the namespace/table identifier (and warehouse/token), and the tables object_store options will come from the catalog server.

With the present changes we still need to pre-populate the ObjectStoreBuilder with the correct options, and only the bucket gets set upon finding the table location (by calling ObjectStoreBuilder::build).

@JanKaul
Copy link
Owner Author

JanKaul commented Mar 25, 2025

You're right, the REST catalog shouldn't have an object_store_builder.

This is supposed to be a refactor PR to lay the groundwork for any further changes. I'll update the REST catalog with credential vending in a separate PR.

@JanKaul JanKaul force-pushed the refactor/object_store_in_table branch from 402afed to bb192f7 Compare March 25, 2025 13:33
@JanKaul JanKaul merged commit cde5cf1 into main Mar 25, 2025
2 checks passed
@JanKaul JanKaul deleted the refactor/object_store_in_table branch April 10, 2025 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce object_store field in Table struct
2 participants