Core: Add metadata table for discovering metadata tables#17134
Core: Add metadata table for discovering metadata tables#17134yangshangqing95 wants to merge 1 commit into
Conversation
44676e5 to
4c79db9
Compare
gaborkaszab
left a comment
There was a problem hiding this comment.
Hi @yangshangqing95 ,
I took a Quick Look and left some comments wrt refactors and the new metadata scan implementation.
A general note: querying static data such as metadata table names through SELECT * FROM catalog.db.table.metadata_tables seems somewhat odd to me. We'd expect the same answer regardless the table we use for the query, right?
Anyway, I recall for Iceberg V4 there are consideration to not allow some of the current metadata table and have new ones because the physical representation of a table changes. So this might make sense to have different answers based on the table's version.
Let's see what others think!
| .map(metadataType -> StaticDataTask.Row.of(metadataType.tableName())) | ||
| .toArray(StaticDataTask.Row[]::new); | ||
|
|
||
| DataFile metadataFile = |
There was a problem hiding this comment.
I randomly checked other metadata tables and they seem to use the following InputFile:
table().io().newInputFile(table().operations().current().metadataFileLocation()),
There was a problem hiding this comment.
That’s a good point. For listing the available metadata tables, at least at this stage, we don’t actually need to read any physical file.
Using a directly constructed empty DataFile has a couple of benefits here:
- It avoids unnecessary file operations.
- It makes the intent clearer in the code, since otherwise it may look like the metadata table information is stored in or read from the table metadata file.
So I think using an empty DataFile is more explicit for this particular metadata table.
There was a problem hiding this comment.
Please follow the pattern other metadata tables are using. See my comment above for constructor vs of()
| .withFileSizeInBytes(0) | ||
| .build(); | ||
|
|
||
| return new StaticDataTask(metadataFile, schema(), scan.schema(), rows); |
There was a problem hiding this comment.
Other metadata tables use StaticDataTask.of()
There was a problem hiding this comment.
same reason as the above one
| } | ||
|
|
||
| private DataTask task(BaseTableScan scan) { | ||
| StaticDataTask.Row[] rows = |
There was a problem hiding this comment.
Checking RefsTable instead of constructing StaticDataTask.Row[]', we can simply use Collectionwith a transform function to convert it to row viaStaticDataTask.Row.of()`
There was a problem hiding this comment.
Hi @gaborkaszab same reason with the usage of empty Datafile, as constructor accept parameter Row[]
StaticDataTask(
DataFile metadataFile,
Schema tableSchema,
Schema projectedSchema,
StructLike[] rows)
There was a problem hiding this comment.
I don't think we are expected to use the constructor from metadata table code. They use StaticDataTask.of() instead.
Hi @gaborkaszab thanks for taking a look and leaving the comments! Regarding the metadata table query, I did consider this initially as well. From both an implementation perspective and the current project structure, I’m still leaning toward exposing it under each table, mainly for a few reasons:
So from that perspective, it may not necessarily be true that all tables should always return the exact same metadata tables. |
f3ccf81 to
c178a29
Compare
gaborkaszab
left a comment
There was a problem hiding this comment.
Thanks for the changes @yangshangqing95 !
Seems way simpler now than initially. Left some further comments, but in the meantime let me try to get additional feedback on the approach, because SELECT * FROM db.tbl.metadata_tables; still feels a bit odd for me, however I get the explanation too.
Maybe @nastra would you mind taking a look?
| return this.lower; | ||
| } | ||
|
|
||
| public static List<String> lowerNames() { |
There was a problem hiding this comment.
I have't found any usage for this in this PR. If I haven't missed anything, please remove.
| POSITION_DELETES, | ||
| METADATA_TABLES; | ||
|
|
||
| private final String lower; |
There was a problem hiding this comment.
I'd leave it to the metadata table code to convert this to lowercase. I'd rather remove this and the getter method too
There was a problem hiding this comment.
Thanks for the review. I was treating the lowercase table name as a property of MetadataTableType, since it is the canonical metadata table name exposed to users and used during resolution. My goal was to avoid duplicating the conversion logic and hard-coded suffixes across metadata table code.
However, I see your point that adding state and accessors to the enum may be broader than necessary for this PR. If you insist I can move the lowercase conversion into the new metadata_tables implementation and remove tableName() / tableNames() from MetadataTableType.
| return new AllEntriesTable(baseTable, metadataTableName); | ||
| case POSITION_DELETES: | ||
| return new PositionDeletesTable(baseTable, metadataTableName); | ||
| case METADATA_TABLES: |
There was a problem hiding this comment.
Would be nice to have some test coverage too for the whole PR. Leaving the comment here to keep track.
There was a problem hiding this comment.
yes there were testings but dropped with some previous modification, re-added.
c5344da to
9786c6e
Compare
9786c6e to
c13a880
Compare
close #17132
Summary
This PR adds a new Iceberg metadata table, metadata_tables, to make available metadata tables discoverable through the existing metadata table mechanism.
For example, in Spark users can query:
Result:
The new metadata table returns the supported metadata table names
This avoids requiring users to know all metadata table names ahead of time from documentation or source code.
Also, replace selected hard-coded metadata table suffixes in tests and metadata table implementations with MetadataTableType#tableName, enhance code robustness.