Remove FILE self-references - #603
Conversation
Key the self-reference AAD on `offset` rather than a derived ordinal, so a reader can resolve a self-reference without decoding the pages it skips. State that the encryption buffer of a self-reference must fit the 4-byte length field, and that larger values must use an external reference. State that the decompressed size is not stored and how readers obtain it. State that an encrypted stored representation is bound to a single column chunk and must not be shared between column chunks.
Align the wording with the validation rules, which state the requirement normatively. Co-authored-by: Isaac
Describe self-reference storage as being anywhere in the file rather than in a page, since the referenced bytes are outside the page data. Drop the forward-looking note about a possible decompressed size field. State that `offset` and `size` cover the encrypted module for an encrypted self-reference, as already stated for the unencrypted case. Describe rather than mandate how the stored bytes are interpreted. Co-authored-by: Isaac
The previous wording described `size` as the size after compression and encryption, which omits the length prefix and nonce that the range also covers. Reference the layout in Encryption.md so the extent is defined in one place. Co-authored-by: Isaac
Drop self-references from the FILE specification. `offset` and `size` now apply only to data referenced by `uri`, and a byte range within the current file cannot be referenced. Storing a large value elsewhere in the same file is better served by out-of-band pages, which can hold the value outside the page data without the FILE type having to define its own compression, encryption, and addressing rules. That will be proposed separately. This reverts the compression and encryption semantics added earlier on this branch, so Encryption.md returns to its state before it. Co-authored-by: Isaac
|
Change of direction: this PR now removes The AAD discussion is what convinced me. Making a self-referenced range a first-class encryption module meant the Out-of-band pages answer all of them once, in the page layer, for every type rather than just What this PR does now:
@rok @wgtmac @danielcweeks @brkyvz — the open threads all point at text that no longer exists, so nothing there needs an answer from you. I have left them unresolved rather than resolving them myself, since the reasoning in them is the reason for this change and worth keeping visible. The parts of your feedback that survive independently of self-references are already in: the aligned resolution table (@brkyvz) and the normative |
wgtmac
left a comment
There was a problem hiding this comment.
I'm fine with removing self-reference. Since this is a spec change after previous vote, perhaps we need yet another vote for this?
Resolve `uri` uniformly as an external reference, including when it names the file that contains it, rather than forbidding a reference to the current file, which a writer or reader cannot check. Allow `inline` together with the locator fields, which must denote the same bytes, and record the locator as provenance. State that the fields of a `FILE` group are ordinary columns and are encoded, compressed, and encrypted like any other column. Co-authored-by: Isaac
A reader that projects the locator fields without `inline` cannot tell whether `inline` was set, so resolving from the locator has to be a valid path. Say so, and say that the value is invalid when the two disagree, without requiring readers to detect it. Co-authored-by: Isaac
|
Parquet.thrift should also be updated to reflect removal of self-reference, currently it states: |
…uet-format#603) Self-references are being removed from the Parquet FILE type, so they are no longer a Delta-specific restriction. FILE is now inline-or-external only, with an absolute uri as Delta's sole added restriction. Co-authored-by: Isaac <no-reply@databricks.com>
…format#603) Co-authored-by: Isaac <no-reply@databricks.com>
The resolution table allowed any locator combination alongside `inline`, which contradicted the validation rules: `offset` still requires `uri` and `size`. Mark those cells as any combination that is valid on its own. Update the FileType comment in parquet.thrift, which still described bytes stored elsewhere in this file. Co-authored-by: Isaac
|
Thanks for catching I checked the rest of the file and that comment was the only place self-references were mentioned outside |
The locator combinations and `inline` are independent, so enumerating them together needed a row per pair and still did not say what `inline` means. Move `inline` to a single cell spanning the table, which states once that it may accompany any combination and holds the same bytes that resolving the rest of the row returns. State that `inline` resolves a value even when its locator is invalid. Co-authored-by: Isaac
This reverts commit 9220368.
The footnote required any locator alongside `inline` to be valid on its own, which read as making a value with only `inline` set invalid, since no locator fields are set in that case. Co-authored-by: Isaac
Nothing can verify that the bytes behind a `uri` match `inline` without fetching and comparing them, so requiring it stated a rule no reader enforces. Keep the part that readers need, which is that either may be read, and leave agreement to producers. Co-authored-by: Isaac
Which path costs less is an implementation matter and does not need to be stated to make the rule clear. Co-authored-by: Isaac
Remove self-references from the
FILEspecification:offsetandsizeapply only to data referenced byuriEncryption.mdreturns to its state on masterA large value that should live elsewhere in the same file is better served by out-of-band pages, which can hold it outside the page data without the
FILEtype defining its own compression, encryption, and addressing rules. That will be proposed separately.This supersedes the earlier direction of this PR, which specified storage inheritance for self-references. The review threads on that approach are left in place for history — see in particular the AAD identity discussion, which is what motivated moving this into the page layer instead.
Discussion: https://lists.apache.org/thread/zrzc7t9fccg92rx3h4fw3ndw3bdo5xr7