feat: metadata access support for table #111
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gty404
reviewed
May 27, 2025
lidavidm
reviewed
May 28, 2025
wgtmac
suggested changes
May 28, 2025
lishuxu
force-pushed
the
feature/my-feature
branch
from
fc2ca57 to
6a26f30
Compare
wgtmac
suggested changes
May 29, 2025
src/iceberg/table.h
Outdated
| /// \brief Return the schema for this table | ||
| virtual const std::shared_ptr<Schema>& schema() const = 0; | ||
| /// \brief Return the schema for this table, return NotFoundError if not found | ||
| virtual Result<std::shared_ptr<Schema>> schema() const = 0; |
There was a problem hiding this comment.
Suggested change
| virtual Result<std::shared_ptr<Schema>> schema() const = 0; | |
| virtual Result<std::shared_ptr<Schema>> Schema() const = 0; |
These functions are no longer trivial accessors. We need to rename them with capitalized initial.
There was a problem hiding this comment.
The method name schema will conflicts with the Schema object name, and since this is actually a lazy getter, I think it can be treated as an accessor.
src/iceberg/table.h
Outdated
| /// \brief Return the partition spec for this table | ||
| virtual const std::shared_ptr<PartitionSpec>& spec() const = 0; | ||
| /// \brief Return the partition spec for this table, return NotFoundError if not found | ||
| virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0; |
There was a problem hiding this comment.
Suggested change
| virtual Result<std::shared_ptr<PartitionSpec>> spec() const = 0; | |
| virtual Result<std::shared_ptr<PartitionSpec>> Spec() const = 0; |
src/iceberg/table.h
Outdated
| /// \brief Return the sort order for this table | ||
| virtual const std::shared_ptr<SortOrder>& sort_order() const = 0; | ||
| /// \brief Return the sort order for this table, return NotFoundError if not found | ||
| virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0; |
There was a problem hiding this comment.
Suggested change
| virtual Result<std::shared_ptr<SortOrder>> sort_order() const = 0; | |
| virtual Result<std::shared_ptr<SortOrder>> SortOrder() const = 0; |
src/iceberg/table.h
Outdated
| /// \brief Return the table's current snapshot | ||
| virtual const std::shared_ptr<Snapshot>& current_snapshot() const = 0; | ||
| /// \brief Return the table's current snapshot, or NotFoundError if not found | ||
| virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0; |
There was a problem hiding this comment.
Suggested change
| virtual Result<std::shared_ptr<Snapshot>> current_snapshot() const = 0; | |
| virtual Result<std::shared_ptr<Snapshot>> CurrentSnapshot() const = 0; |
wgtmac
reviewed
May 30, 2025
src/iceberg/table.cc
Outdated
| BaseTable::BaseTable(std::string name, std::shared_ptr<TableMetadata> metadata) | ||
| : name_(std::move(name)), metadata_(std::move(metadata)) { | ||
| if (!metadata_) { | ||
| throw std::invalid_argument("Table metadata cannot be null"); |
src/iceberg/table.cc
Outdated
| void BaseTable::InitSchema() const { | ||
| for (const auto& schema : metadata_->schemas) { | ||
| if (schema->schema_id()) { | ||
| schemas_map_.emplace(schema->schema_id().value(), schema); |
There was a problem hiding this comment.
I think these changes are still required
lishuxu
changed the title
lishuxu
force-pushed
the
feature/my-feature
branch
2 times, most recently
from
50c93a4 to
6aef4b4
Compare
added 13 commits
June 3, 2025 11:32
lishuxu
force-pushed
the
feature/my-feature
branch
from
5304adf to
345355a
Compare
lishuxu
changed the title
wgtmac
suggested changes
Jun 20, 2025
src/iceberg/table.cc
Outdated
| } | ||
|
|
||
| const std::shared_ptr<PartitionSpec>& Table::spec() const { | ||
| std::call_once(init_partition_spec_once_, [this]() { |
There was a problem hiding this comment.
I'd suggest to follow similar implementation as Table::schema(). In this function, we can cache and return PartitionSpec::Unpartitioned() when missing.
src/iceberg/table.h
Outdated
| /// \brief Return the table's current snapshot | ||
| virtual const std::shared_ptr<Snapshot>& current_snapshot() const = 0; | ||
| /// \brief Return the table's current snapshot, return null if not found | ||
| std::shared_ptr<Snapshot> current_snapshot() const; |
There was a problem hiding this comment.
Suggested change
| std::shared_ptr<Snapshot> current_snapshot() const; | |
| const std::shared_ptr<Snapshot>& current_snapshot() const; |
Every table should have at least one snapshot. I think we can safely return a reference.
src/iceberg/table.cc
Outdated
| } | ||
|
|
||
| const std::unordered_map<int32_t, std::shared_ptr<Schema>>& Table::schemas() const { | ||
| std::call_once(init_schemas_once_, [this]() { |
There was a problem hiding this comment.
What about removing the once flag and use std::shared_ptr<std::unordered_map<int32_t, std::shared_ptr<Schema>>> instead?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.