stefan-grafberger commented on code in PR #635: URL: https://github.com/apache/parquet-format/pull/635#discussion_r4466297823
########## LogicalTypes.md: ########## @@ -796,10 +800,8 @@ compressed, and encrypted like any other column, `inline` included. does not resolve and is invalid. * `size` must be set whenever `offset` is set. A value that sets `offset` without `size` is invalid. -* If `inline` and a locator are both set, a reader may resolve the value from either. - Producers are expected to write the same bytes in both, but a reader is not required - to check this and may return the bytes of either. Producers may treat `inline` and the - locator fields as mutually exclusive. +* If `inline` is set, it supplies the bytes for readers; producers may treat `inline` and + the locator fields as mutually exclusive. Review Comment: Personally, I think keeping this equality guarantee is a bit risky, because only a part of it is enforceable: - Enforceable: at the moment a writer writes the value, it can ensure that inline is a copy of the bytes at uri. - Not enforceable: that they stay equal afterwards. "External" objects usually live in user-owned storage that the engine writing the Parquet file doesn't control, so anyone can overwrite or delete the object at uri later. We also discussed this internally and concluded that, with the current wording, it isn't safe for us to write uri and inline together. I'd expect other implementations to face the same question, so I think it's best if the spec here only asks writers for what they can guarantee. If an engine can keep inline and uri equal, for example because it controls the storage, it can still do so with the updated wording. The Parquet spec doesn't address whether the object at uri has to exist. But the Iceberg spec, for example, which defines its file type by this Parquet logical type, says explicitly: "A file entry that references a uri does not guarantee its existence." [1] If writers can't ensure the object stays present, requiring it to stay equal to an earlier state seems like an even stronger promise. Stronger guarantees would need a table format that owns the referenced files (which the Iceberg community is discussing as a future optional feature). Even then, the Parquet spec would still need to cover files written outside such table formats, or in tables without such ownership semantics, so I don't think it should ask writers for that promise. That's why the PR states only what writers can ensure: inline is the value, and the locator and checksum record where it was copied from at write time. What you're proposing would already be much better than not changing it at all. However, if we keep an equality sentence, I'd suggest scoping it to what writers can guarantee, e.g. "inline is a copy of the bytes referenced by the locator at the time the value is written". I think the PR in its current version already implies that. What do you think? [1] https://github.com/apache/iceberg/blob/main/format/spec.md#file-type -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
