feat(inspect): Add base class for metadata table support#607
Conversation
wgtmac
left a comment
wgtmac
left a comment
There was a problem hiding this comment.
Thanks for working on this! I've left some preliminary comments.
| class UpdateSortOrder; | ||
| class UpdateStatistics; | ||
|
|
||
| /// \brief Metadata tables. |
| /// | ||
| /// Once a table scan builder is created, it can be refined to project columns and | ||
| /// filter data. | ||
| Result<std::unique_ptr<TableScanBuilder>> NewScan() const; |
|
|
||
| std::shared_ptr<Table> source_table_; | ||
| std::string uuid_; | ||
| std::shared_ptr<Schema> schema_; |
There was a problem hiding this comment.
Member variables here and below are not necessary
There was a problem hiding this comment.
The purpose of those variables is that the MetadataTable won't have the same uuid/schema/etc as the source table. If we remove them, the MetadataTable will return those properties directly from the source table's TableMetadata.
| const std::string& uuid() const { return uuid_; } | ||
|
|
||
| /// \brief Returns the schema for this table, return NotFoundError if not found | ||
| Result<std::shared_ptr<Schema>> schema() const { return schema_; } |
There was a problem hiding this comment.
Let's remove these redefinitions here and below
| /// | ||
| /// \param table The source table | ||
| /// \return A SnapshotsTable exposing all snapshots or error status | ||
| static Result<std::unique_ptr<SnapshotsTable>> GetSnapshotsTable( |
There was a problem hiding this comment.
It looks cumbersome to use a function per type. Can we use enum metadata type or canonical metadata table name suffix instead to return a std::unique_ptr<Table> instead?
There was a problem hiding this comment.
BTW, do we want to support the case when users directly create a metadata table without having a table object? For example, users may directly create a metadata table from catalog by calling LoadTable and providing full table identifier with metadata table suffix.
There was a problem hiding this comment.
BTW, do we want to support the case when users directly create a metadata table without having a table object? For example, users may directly create a metadata table from catalog by calling LoadTable and providing full table identifier with metadata table suffix.
This breaks the assumption that MetadataTables are companion objects to an existing table. Allowing this might call for unnecessary consideration (for example, how to read the history/snapshots of a non-existent table).
|
Please rebase to resolve conflicts. Let me know when it is ready to review again. |
2 participants
This PR kicks off the implementation of table inspection support.
BaseMetadataTableclass (without scan support for now).MetadataTableFactoryas an entry point for creating metadata tables.SnapshotsTableandHistoryTableas example impl.