pitrou commented on code in PR #585:
URL: https://github.com/apache/parquet-format/pull/585#discussion_r3500070984


##########
src/main/thrift/parquet.thrift:
##########
@@ -468,6 +468,21 @@ struct GeographyType {
   2: optional EdgeInterpolationAlgorithm algorithm;
 }
 
+/**
+ * File logical type annotation
+ *
+ * Annotates a group that represents a reference to an external file.
+ * The group must contain the following fields identified by name:
+ *   - path (STRING, required): an opaque string path to the file (e.g. 
s3://bucket/file.jpg)

Review Comment:
   If it's meant to be a URI, can we mandate it and call the field "uri" 
instead?



##########
src/main/thrift/parquet.thrift:
##########
@@ -468,6 +468,21 @@ struct GeographyType {
   2: optional EdgeInterpolationAlgorithm algorithm;
 }
 
+/**
+ * File logical type annotation
+ *
+ * Annotates a group that represents a reference to an external file.
+ * The group must contain the following fields identified by name:
+ *   - path (STRING, required): an opaque string path to the file (e.g. 
s3://bucket/file.jpg)
+ *   - size (INT64, optional): the length of the content in bytes; must be 
zero or positive
+ *   - offset (INT64, optional): byte offset for range reads; if provided, 
size must also be provided
+ *   - etag (STRING, optional): eTag from the storage system for staleness 
detection

Review Comment:
   Isn't this a bit specific? Do some storage systems support something other 
than a ETag? Sooner or later, people will want to put other kinds of metadata, 
such as Last-Modified, Content-Type...



##########
src/main/thrift/parquet.thrift:
##########
@@ -468,6 +468,21 @@ struct GeographyType {
   2: optional EdgeInterpolationAlgorithm algorithm;
 }
 
+/**
+ * File logical type annotation
+ *
+ * Annotates a group that represents a reference to an external file.
+ * The group must contain the following fields identified by name:
+ *   - path (STRING, required): an opaque string path to the file (e.g. 
s3://bucket/file.jpg)

Review Comment:
   Why would a Parquet file reference itself recursively?



##########
LogicalTypes.md:
##########
@@ -635,7 +635,76 @@ The type has two type parameters:
 
 The sort order used for `GEOGRAPHY` is undefined. When writing data, no min/max
 statistics should be saved for this type and if such non-compliant statistics
-are found during reading, they must be ignored. 
+are found during reading, they must be ignored.
+
+### FILE
+
+`FILE` annotates a group that represents a reference to an external file, 
along with
+the minimum metadata required to read it. It is intended for use cases such as
+storing file inventories, manifests, and unstructured data references (e.g., 
images
+or audio files stored in object storage).
+
+The annotated group must contain the following fields, identified by name. 
Field IDs
+may also be used for projection:
+
+| Field    | Type   | Required |
+|----------|--------|----------|
+| `path`   | STRING | Yes      |
+| `size`   | INT64  | No       |
+| `offset` | INT64  | No       |
+| `etag`   | STRING | No       |
+
+#### Fields
+
+##### path
+
+An opaque path string to the referenced file (e.g., `s3://bucket/file.jpg`). 
No special
+encoding (e.g., URI encoding) is applied. This is the only required field.

Review Comment:
   The example shows a URI but this mandates no URI encoding??



##########
src/main/thrift/parquet.thrift:
##########
@@ -468,6 +468,21 @@ struct GeographyType {
   2: optional EdgeInterpolationAlgorithm algorithm;
 }
 
+/**
+ * File logical type annotation
+ *
+ * Annotates a group that represents a reference to an external file.
+ * The group must contain the following fields identified by name:
+ *   - path (STRING, required): an opaque string path to the file (e.g. 
s3://bucket/file.jpg)
+ *   - size (INT64, optional): the length of the content in bytes; must be 
zero or positive
+ *   - offset (INT64, optional): byte offset for range reads; if provided, 
size must also be provided

Review Comment:
   Why would there be an offset here? 



-- 
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]

Reply via email to