Introduces a new LogicalType: FILE#585
Conversation
emkornfield
left a comment
There was a problem hiding this comment.
Looks reasonable to me a few minor comments for clarification.
etseidl
left a comment
There was a problem hiding this comment.
Just a few questions I have after reviewing the Rust implementation.
| The referenced bytes are compressed with the same `CompressionCodec` as the one | ||
| specified for the `inline` column. |
There was a problem hiding this comment.
Let's make this clearer.
| The referenced bytes are compressed with the same `CompressionCodec` as the one | |
| specified for the `inline` column. | |
| The bytes referenced by a self-reference are compressed with the same `CompressionCodec` as the one specified for the `inline` column. |
There was a problem hiding this comment.
Should we point out CompressionCodec can differ per page?
There was a problem hiding this comment.
@alkis thoughts? Is there a reason that the data needs to be compressed additionally? Most formats may be optimal anyway.
There was a problem hiding this comment.
The reason is because we are making the assumption that the type of data stored in the file column is homogeneous. I argue that's a good assumption. If this column contains text, there is little reason to assume it will be a string when small and an image when large. Ergo if the writer chose to compress the column it should compress the self-referenced packed blobs too.
I wanted to make all packed references compressed as well but there was some pushback for that with the argument that external may be referenced by different parquet files and it may choose its own compression scheme (or none at all). I can see arguments both ways - it is a tradeoff.
Making all packed references inherit the CompressionCodec makes the spec more consistent.
Making only self-references inherit the CompressionCodec is friendlier to shared external references.
We pick one of the two, it is a tradeoff.
There was a problem hiding this comment.
Should we point out CompressionCodec can differ per page?
sfc-gh-sgrafberger
left a comment
There was a problem hiding this comment.
Thank you @brkyvz! Looks good to me.
…a-io#585) Align with the latest apache/parquet-format#585: - size must be set whenever offset is set; a self-reference (no path) must set offset, and therefore size. Drop the now-invalid [offset, EOF) and [0, size) self-reference modes and the external [offset, EOF) mode from the resolution table; add explicit invalid rows. - Define "set" (present, non-null, non-empty for strings) and allow sparse group definitions (a group need only define the fields it uses); add an inline-only example group. - Fields matched case-sensitively by name; field IDs "if they exist". - Readers should ignore unknown checksum algorithms. Follows the consistent prose intent of PR delta-io#585; note its resolution table still lists an [offset, EOF) row that contradicts its own validation section (offset requires size) -- to be raised on the Parquet PR. Co-authored-by: Isaac
| * A self-reference (`path` not set) must set `offset`. A value with neither `path` nor | ||
| `offset` set (and not `inline`) does not resolve and is invalid. | ||
| * `size` must be set whenever `offset` is set. A value that sets `offset` without `size` | ||
| is invalid. Because a self-reference must set `offset`, it must also set `size`. |
There was a problem hiding this comment.
How should implementations behave if offset is set but size is not? Are they required to fail?
There was a problem hiding this comment.
Ideally fail at write time, but is there a precedent for enforcing that? From your past comments it seems like we don't necessarily enforce contents of columns. Invalid files can be treated as null files in my opinion if we can't enforce it
There was a problem hiding this comment.
A "must" requirement is sufficient for writers. I think that requirement implies a failure at write time because otherwise the requirements are violated.
I'm more concerned about read time, which is why I'm asking. If this is invalid, what is the expected reader behavior? In the offset=null case, size is understood to be the length of the file. That doesn't seem right, but my guess is that readers will produce bytes up to the end of the file unless you specifically state that this should fail.
rdblue
left a comment
There was a problem hiding this comment.
I left a few comments for clarification, but I think that this is ready either way.
| the current file, so a file containing self-references is renamed or relocated as a | ||
| single unit. | ||
|
|
||
| The bytes referenced by a self-reference are compressed with the same `CompressionCodec` |
There was a problem hiding this comment.
The does not look right. Same column can have different codecs across different row groups. V2 pages can even decide whether to compress individually. Even if all row groups of the same column use the same codec, how can we tell whether self-referenced data is compressed or not?
There was a problem hiding this comment.
reworded to
The bytes referenced by a self-reference use the `CompressionCodec`
defined by the `inline` column chunk's `ColumnMetadata`.
is that better @wgtmac ?
There was a problem hiding this comment.
I may have missed the relevant discussion on this so please correct me if I was wrong.
CompressionCodec defined by a ColumnMetadata is to compress all data pages of that column chunk in that specific row group. The description here means that we enforce all inline blocks referenced by this FILE column to always use the same CompressionCodec in the same row group? Is this too restrictive? Why can't we choose them to be uncompressed (like bloom filter) or compressed by a different scheme?
For other non-self-referenced cases, we can deduce compression codec based on content_type field, right? Shouldn't we do the same thing?
There was a problem hiding this comment.
For other non-self-referenced cases, we can deduce compression codec based on content_type field, right? Shouldn't we do the same thing?
The intent of the field is to capture the content type of the data for filters like "find all PNG images". I don't expect this to describe compression because that wouldn't be very useful (all application/zip, for example).
I think the content_type field would behave like the HTTP Content-Type header that is the original media type before compression. HTTP uses a separate Content-Encoding header for compression.
This leaves the question of how to determine the equivalent of Content-Encoding for external references. We should probably spend some time considering that more.
The description here means that we enforce all inline blocks referenced by this FILE column to always use the same CompressionCodec in the same row group? Is this too restrictive? Why can't we choose them to be uncompressed (like bloom filter) or compressed by a different scheme?
Yes, I think that reading is accurate. Looks like this is a way to determine compression, but only for blobs that are located within the Parquet file itself as a self-reference.
I don't think that this is too restrictive since generic compression is typically uniform for files. I don't expect people to customize the compression used for these. We want compression to be possible (mandating uncompressed doesn't seem like a good choice), but we don't need to track it per value.
| ``` | ||
|
|
||
| Because every field is optional, a group need only define the fields it uses. A group | ||
| whose values are always stored inline may define just `inline`: |
There was a problem hiding this comment.
Why defined content_type below but here says just inline? Seems inconsistent.
There was a problem hiding this comment.
I don't see this is addressed. At least inline_file struct below still defines content_type.
|
@rdblue @wgtmac @mapleFU @RussellSpitzer Thank you for the feedback! Addressed your comments. Hopefully things are clearer now! Let me know if they aren't |
| | Algorithm | Encoding | Notes | | ||
| |-----------|---------------|----------------------------------------------------------| | ||
| | `ETAG` | opaque | the object-store eTag, not recomputable | | ||
| | `MD5` | lowercase hex | as defined in RFC 6151 represented as 32 hex characters | |
There was a problem hiding this comment.
I would propose we reference RFC 1321 (The MD5 Message-Digest Algorithm) over RFC 6151 (Updated Security Considerations for the MD5 Message-Digest and the HMAC-MD5 Algorithms).
| | `MD5` | lowercase hex | as defined in RFC 6151 represented as 32 hex characters | | |
| | `MD5` | lowercase hex | as defined in RFC 1321 represented as 32 hex characters | |
| | `CRC32` | lowercase hex | as defined in RFC 3385, represented as 8 hex characters | | ||
| | `CRC32C` | lowercase hex | as defined in RFC 9260, represented as 8 hex characters | |
There was a problem hiding this comment.
These might fit better - I am in no way an expert on CRC32(c) RFC 2083, RFC 3385, but we would want to specify exact alghoritms used.
| | `CRC32` | lowercase hex | as defined in RFC 3385, represented as 8 hex characters | | |
| | `CRC32C` | lowercase hex | as defined in RFC 9260, represented as 8 hex characters | | |
| | `CRC32` | lowercase hex | as defined in RFC 2083, represented as 8 hex characters | | |
| | `CRC32C` | lowercase hex | as defined in RFC 3385, represented as 8 hex characters | |
| `offset` set (and not `inline`) does not resolve and is invalid. | ||
| * `size` must be set whenever `offset` is set. A value that sets `offset` without `size` | ||
| is invalid. Because a self-reference must set `offset`, it must also set `size`. | ||
| * If `inline` is set, it supplies the bytes readers; producers may treat `inline` and the |
There was a problem hiding this comment.
| * If `inline` is set, it supplies the bytes readers; producers may treat `inline` and the | |
| * If `inline` is set, it supplies the bytes; producers may treat `inline` and the |
| | set | – | – | – | the inline bytes | | ||
| | – | set | – | – | whole external file at `path` | | ||
| | – | set | set | - | invalid | | ||
| | – | set | – | set | external `path`, `[0, size)` | | ||
| | – | set | set | set | external `path`, `[offset, offset + size)` | | ||
| | – | - | set | - | invalid | | ||
| | – | - | - | set | invalid | | ||
| | – | – | set | set | this file, `[offset, offset + size)` (self-reference) | | ||
| | – | – | – | – | nothing — invalid | |
There was a problem hiding this comment.
nit: this table uses both dash (-) and em dash (–) to indicate null input. Let's just use dash?.
Rationale for this change
Introduces a new type called File as a typed FileReference. The design document is here.
The motivation is as follows:
What changes are included in this PR?
Introduces the specification for FileType.
Do these changes have PoC implementations?
Yes: