divjotarora commented on code in PR #3608:
URL: https://github.com/apache/parquet-java/pull/3608#discussion_r3393851721
##########
parquet-column/src/main/java/org/apache/parquet/schema/LogicalTypeAnnotation.java:
##########
@@ -1221,6 +1231,55 @@ public boolean equals(Object obj) {
}
}
+ /**
+ * File logical type annotation. Annotates a group (struct) that represents
a reference to
+ * an external file. The group must contain the following fields by name:
+ * <ul>
+ * <li>{@code path} (required): STRING - the path/URI of the file</li>
+ * <li>{@code size} (optional): INT64 - size of the file content in
bytes</li>
+ * <li>{@code offset} (optional): INT64 - byte offset within the file; if
present, size must be present</li>
+ * <li>{@code etag} (optional): STRING - opaque identifier for the file
version</li>
+ * </ul>
+ * No optional fields with names other than the above are permitted.
Review Comment:
```suggestion
* No fields other than the above are permitted.
```
##########
parquet-column/src/main/java/org/apache/parquet/schema/Types.java:
##########
@@ -821,12 +821,34 @@ public THIS addFields(Type... types) {
@Override
protected GroupType build(String name) {
if (newLogicalTypeSet) {
+ if (logicalTypeAnnotation instanceof
LogicalTypeAnnotation.FileLogicalTypeAnnotation) {
+ validateFileTypeFields(name, fields);
+ }
return new GroupType(repetition, name, logicalTypeAnnotation, fields,
id);
} else {
return new GroupType(repetition, name, getOriginalType(), fields, id);
}
}
+ private static void validateFileTypeFields(String name, List<Type> fields)
{
Review Comment:
Where do we validate:
1. If `offset` is present, `size` must be present
2. `offset` and `size` must be non-negative (note spec doesn't technically
say `offset` must be >= 0, but I left a suggestion on that change to add it
##########
parquet-column/src/test/java/org/apache/parquet/schema/TestTypeBuildersWithLogicalTypes.java:
##########
Review Comment:
We should add tests for validity of size/offset fields, both their values
(>= 0) and valid/invalid combinations
##########
parquet-column/src/main/java/org/apache/parquet/schema/Types.java:
##########
@@ -821,12 +821,34 @@ public THIS addFields(Type... types) {
@Override
protected GroupType build(String name) {
if (newLogicalTypeSet) {
+ if (logicalTypeAnnotation instanceof
LogicalTypeAnnotation.FileLogicalTypeAnnotation) {
+ validateFileTypeFields(name, fields);
+ }
return new GroupType(repetition, name, logicalTypeAnnotation, fields,
id);
} else {
return new GroupType(repetition, name, getOriginalType(), fields, id);
}
}
+ private static void validateFileTypeFields(String name, List<Type> fields)
{
+ boolean hasPath = false;
+ for (Type field : fields) {
+ String fieldName = field.getName();
+ if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.PATH_FIELD.equals(fieldName)) {
+ Preconditions.checkArgument(
+ field.getRepetition() == Type.Repetition.REQUIRED,
+ "FILE type field 'path' must be REQUIRED in group '%s'",
+ name);
+ hasPath = true;
+ } else if
(!LogicalTypeAnnotation.FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES.contains(fieldName))
{
+ throw new IllegalArgumentException(
+ "FILE type group '" + name + "' contains unrecognized field '" +
fieldName
+ + "'. Valid fields are: path, size, offset, etag");
Review Comment:
nit: stringify `FileLogicalTypeAnnotation.OPTIONAL_FIELD_NAMES` and use it
in the error to avoid drift if we add new fields?
--
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]
