feat: add schema update to table metadata builder #437
Conversation
gty404
force-pushed
the
update-schema
branch
from
7836c3e to
62960f3
Compare
gty404
force-pushed
the
update-schema
branch
from
012079a to
16e797e
Compare
| } | ||
| } | ||
|
|
||
| // TODO(GuoTao.yu): Check default values when they are supported |
There was a problem hiding this comment.
Do you think we need to add the Java Schema.validateIdentifierField and call it from here?
cc @WZhuo
There was a problem hiding this comment.
I think Schema.validateIdentifierField should be called in Schema::Make, I can do it in a seperate PR
There was a problem hiding this comment.
This method is equivalent to java's checkCompatibility, which only checks types and default values.
| // if the schema already exists, use its id; otherwise use the highest id + 1 | ||
| auto new_schema_id = metadata_.current_schema_id.value_or(Schema::kInitialSchemaId); | ||
| for (auto& schema : metadata_.schemas) { | ||
| auto schema_id = schema->schema_id().value_or(Schema::kInitialSchemaId); |
There was a problem hiding this comment.
Do you think we need to change Schema::schema_id to use int32_t instead of std::optional<int32_t>? It seems that we have to deal with nullopt in all places. PartitionSpec and SortOrder do not use optional for ids and Java schema uses 0 as the default. Of course we can do this in a separate PR.
There was a problem hiding this comment.
Yes, we should give a default value when parsing the metadata JSON for an iceberg v1 table, and other fields should be handled similarly.
4 participants
No description provided.