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?
|
@etseidl @sfc-gh-sgrafberger @alkis @rdblue addressed your comments, can you please take another look? |
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
| #### Fields | ||
|
|
||
| For the descriptions below, a field is *set* when it is present in the `FILE` group | ||
| and its value is non-null (and, for string fields, non-empty). A field is *not set* |
There was a problem hiding this comment.
Nit: we should be clear that implementations are not expected to treat empty strings as null.
|
|
||
| ##### content_type | ||
|
|
||
| The media type (MIME type) of the resolved bytes (e.g., `image/png`). |
There was a problem hiding this comment.
Minor: would be nice to have a reference like the URI reference. I thought that we also wanted to have an assumed MIME type, application/octet-stream? I'm fine if it isn't there, but I remember it coming up.
| | `SHA-256` | e.g. S3 additional checksums | | ||
|
|
||
| `checksum` applies to the resolved bytes, except for `ETAG`, which is the | ||
| object-store eTag for the whole file referenced by `path`. |
There was a problem hiding this comment.
An example or a few examples would help here because of the use of base64 in the first paragraph.
Are we really going to use base64 for formats that are normally hexadecimal like etags?
| runs to the end of the referenced file. | ||
|
|
||
| A self-reference points within the same Parquet file using `offset` and `size` (both | ||
| required); the bytes are written between column chunks and are not otherwise referenced by |
There was a problem hiding this comment.
I don't think that we should specify where bytes must be written. Other restrictions apply, but it would be reasonable to place these just before the indexes and footer as well.
|
|
||
| A self-reference points within the same Parquet file using `offset` and `size` (both | ||
| required); the bytes are written between column chunks and are not otherwise referenced by | ||
| the footer. A self-reference is when `path` is not set, never an absolute path back to |
There was a problem hiding this comment.
The phrase "never an absolute path back to the current file" sounds like you're saying "path must not be an absolute reference to the current file and must instead be unset". If that's true, I'd use a more clear phrasing to instruct people to replace self-references with null.
|
|
||
| * A value must resolve to some referenced data. It resolves only if `inline`, `path`, or | ||
| `offset` is set; if none of them are set, the value does not resolve and is invalid, even | ||
| if `size` is set. Use column nullability to represent a null value. |
There was a problem hiding this comment.
Use column nullability to represent a null value
But empty string is considered unset?
| * 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?
| * `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; producers may instead treat `inline` and the | ||
| locator fields as mutually exclusive. |
There was a problem hiding this comment.
Readers may not because locator fields may be persisted to record provenance?
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.
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: