Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions open-api/rest-catalog-open-api.py
Original file line number Diff line number Diff line change
Expand Up @@ -1516,6 +1516,10 @@ class ViewMetadata(BaseModel):


class AddSchemaUpdate(BaseUpdate):
"""
Adds a schema to the table. The field IDs in `schema` are assigned by the client. The client assigns them so that the schema stays consistent with the data files, partition specs, and sort orders it writes or co-commits, which reference fields by their field ID (`StructField.id`). Servers SHOULD preserve the field IDs submitted by the client on add-schema, as reassigning them would require re-deriving and rewriting every such reference, including in already-written data files.
"""
Comment thread
sungwy marked this conversation as resolved.

action: Literal['add-schema']
schema_: Schema = Field(..., alias='schema')
last_column_id: int | None = Field(
Expand Down Expand Up @@ -1636,6 +1640,10 @@ class CommitTransactionRequest(BaseModel):


class CreateTableRequest(BaseModel):
"""
Table creation request. The field IDs in `schema` are assigned by the client, but because a newly created table has no committed data, a server MAY (re)assign them when creating the table.
"""
Comment on lines +1643 to +1645

name: str
location: str | None = None
schema_: Schema = Field(..., alias='schema')
Expand Down
10 changes: 10 additions & 0 deletions open-api/rest-catalog-open-api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -3120,6 +3120,13 @@ components:
AddSchemaUpdate:
allOf:
- $ref: '#/components/schemas/BaseUpdate'
description:
Adds a schema to the table. The field IDs in `schema` are assigned by the client. The client
assigns them so that the schema stays consistent with the data files, partition specs, and
sort orders it writes or co-commits, which reference fields by their field ID
(`StructField.id`). Servers SHOULD preserve the field IDs submitted by the client on

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I made this SHOULD for now, but my preference is to make this a MUST.

The invariant we need to preserve in the catalog server is that the field IDs that are committed are consistent with the constructs (partition spec, sort order, data files) that are committed together with it.

The only way to ensure that invariant are:

preserve the field IDs provided by the client (assuming that the client guarantees consistency within its commit)
reassign field IDs as it wants, and check and rewrite the data files with the new field IDs if needed
I don't think (2) is practical because the data isn't sent over in the payload of the AddSchemaUpdate in CommitTableRequest, only the pointers to it are (through chain of pointers). A server that holds storage credentials could step outside the commit path to scan and rewrite every affected file, but that's an O(data) operation on a metadata commit.

Making (1) a part of the protocol seems practical, reflects the current state of catalog server implementations, and makes the behavior verifiable via conformance tests (RCK can assert it on round-trip)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd argue that this should be a MUST.

Let's say that I'm trying to add a new column and then immediately rename it in the same update (yes, this is a silly example)

  • AddSchemaUpdate adds the new column. User specifies this as ID 10, but catalog ignores it and uses ID 20 instead
  • In that same update, I attempt to add a new schema with AddSchemaUpdate. In that schema, I attempt to rename the column on ID 10. But, the catalog has already renamed it to ID 20.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I prefer "must" (or MUST if you must).

I don't see a good reason for a service to reassign at this point and the client may know something the server does not. For instance, un-deleting a column in existing data files. This isn't a very strong preference though.

add-schema, as reassigning them would require re-deriving and rewriting every such reference,
including in already-written data files.
required:
- action
- schema
Expand Down Expand Up @@ -3930,6 +3937,9 @@ components:

CreateTableRequest:
type: object
description:
Table creation request. The field IDs in `schema` are assigned by the client, but because a newly
created table has no committed data, a server MAY (re)assign them when creating the table.
Comment on lines +3940 to +3942

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe we should have a general statement like this that the service is allowed to do things like reassignment, but its actions must not alter the table data. This could unify both cases because it is allowed to rewrite in both cases and the "must not alter table data" is invariant everywhere.

required:
- name
- schema
Expand Down
Loading