-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Spec: Clarify field id handling in AddSchemaUpdate and CreateTableRequest
#17137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.