Skip to content

Remove FILE self-references - #603

Merged
danielcweeks merged 19 commits into
apache:masterfrom
alkis:file-self-reference-compression
Aug 27, 2026
Merged

Remove FILE self-references#603
danielcweeks merged 19 commits into
apache:masterfrom
alkis:file-self-reference-compression

Conversation

@alkis

@alkis alkis commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Remove self-references from the FILE specification:

  • offset and size apply only to data referenced by uri
  • a byte range within the current file cannot be referenced
  • drop the compression and encryption semantics this PR previously specified, so Encryption.md returns to its state on master
  • leave inline values and external references unaffected

A 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 FILE type 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

@alkis alkis changed the title Specify compression for FILE self-references Specify storage inheritance for FILE self-references Jul 30, 2026
Comment thread LogicalTypes.md
Comment thread LogicalTypes.md Outdated
Comment thread Encryption.md Outdated
Comment thread Encryption.md Outdated
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.
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
alkis added 3 commits August 6, 2026 18:27
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
@alkis
alkis requested a review from wgtmac August 13, 2026 16:44
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
@alkis alkis changed the title Specify storage inheritance for FILE self-references Remove FILE self-references Aug 18, 2026
@alkis

alkis commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Change of direction: this PR now removes FILE self-references instead of specifying their storage semantics (d022d27).

The AAD discussion is what convinced me. Making a self-referenced range a first-class encryption module meant the FILE type had to define its own compression block, its own encryption module, its own AAD identity, and its own size accounting — a parallel storage mechanism living next to pages but not being one. Every review comment here was a symptom of that: whether the range is bound to a page, what its identity is for AAD, whether the size is pre- or post-encryption, whether the decompressed size needs storing. Those questions only exist because the bytes sit outside any page while still needing page-like machinery.

Out-of-band pages answer all of them once, in the page layer, for every type rather than just FILE. A large value gets stored in a page that lives outside the normal page sequence, and it inherits compression, encryption, AAD, and size accounting from the existing page rules with nothing new to specify. I will send that as a separate proposal.

What this PR does now:

  • offset and size apply only to data referenced by uri
  • a byte range within the current file cannot be referenced
  • Encryption.md returns to its state on master
  • inline values and external references are unchanged

@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 size wording (@danielcweeks).

@etseidl etseidl left a comment

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 like removing the self-reference, thanks @alkis.

Comment thread LogicalTypes.md Outdated

@wgtmac wgtmac left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm fine with removing self-reference. Since this is a spec change after previous vote, perhaps we need yet another vote for this?

Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated

@rok rok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the simplification!
Minor comment about encryption.
Re: compression I suppose without ranges it's pretty straight forward.

Comment thread LogicalTypes.md Outdated
@rok

rok commented Aug 19, 2026

Copy link
Copy Markdown
Member

I'm not a fan of process, but self-references and byte ranges were formally voted on and approved as part of #585. This PR now removes that functionality and we should probably call a formal vote for it.

edit: I see @wgtmac already proposed this.

alkis added 2 commits August 19, 2026 08:32
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
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
@rok

rok commented Aug 19, 2026

Copy link
Copy Markdown
Member

Parquet.thrift should also be updated to reflect removal of self-reference, currently it states:

 * Annotates a group that represents a reference to a file, or to a range of
 * bytes that may be stored inline, elsewhere in this file, or in an external
 * file.

dejankrak-db added a commit to dejankrak-db/delta that referenced this pull request Aug 19, 2026
…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>
dejankrak-db added a commit to dejankrak-db/delta that referenced this pull request Aug 19, 2026
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
@alkis

alkis commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for catching parquet.thrift — I had only looked at the two Markdown files. Updated in ed8bb0b:

 * Annotates a group that represents a reference to a file, or to a range of
 * bytes that may be stored inline or in an external file.

I checked the rest of the file and that comment was the only place self-references were mentioned outside LogicalTypes.md and Encryption.md. No Thrift structures change in this PR.

alkis added 2 commits August 20, 2026 09:14
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
Comment thread LogicalTypes.md Outdated
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
Comment thread LogicalTypes.md Outdated
Comment thread LogicalTypes.md Outdated
alkis added 2 commits August 24, 2026 16:50
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
@danielcweeks
danielcweeks merged commit e94a5d0 into apache:master Aug 27, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants