divjotarora commented on code in PR #3608:
URL: https://github.com/apache/parquet-java/pull/3608#discussion_r4131471049
##########
parquet-column/src/main/java/org/apache/parquet/schema/Types.java:
##########
@@ -847,12 +847,115 @@ 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 hasUri = false;
+ boolean hasOffset = false;
+ boolean hasSize = false;
+ boolean hasInline = false;
+ for (Type field : fields) {
+ String fieldName = field.getName();
+ if
(!LogicalTypeAnnotation.FileLogicalTypeAnnotation.FIELD_NAMES.contains(fieldName))
{
+ throw new IllegalArgumentException("FILE type group '" + name + "'
contains unrecognized field '"
+ + fieldName + "'. Valid fields are: "
+ + String.join(", ",
LogicalTypeAnnotation.FileLogicalTypeAnnotation.FIELD_NAMES));
+ }
+ Preconditions.checkArgument(
+ field.isPrimitive() && field.getRepetition() ==
Type.Repetition.OPTIONAL,
+ "FILE type field '%s' must be an optional primitive in group '%s'",
+ fieldName,
+ name);
+ validateFileTypeFieldPhysicalType(name, field.asPrimitiveType());
+ if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.URI_FIELD.equals(fieldName)) {
+ hasUri = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.OFFSET_FIELD.equals(fieldName))
{
+ hasOffset = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.SIZE_FIELD.equals(fieldName)) {
+ hasSize = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.INLINE_FIELD.equals(fieldName))
{
+ hasInline = true;
+ }
+ }
+ // A value resolves to bytes only via inline storage or an external
reference, so a group must
+ // declare at least one of `inline` or `uri`. A group declaring neither
— even if it declares
+ // `offset` or `size` — can never produce a resolvable value, so reject
it at schema-build
+ // time.
+ Preconditions.checkArgument(
+ hasInline || hasUri,
+ "FILE type group '%s' must declare at least one of 'inline' or
'uri'; a value resolves to "
+ + "bytes only via inline storage or an external reference, so a
group declaring "
+ + "neither can never produce a valid value",
+ name);
+ // `offset` locates a byte range within the external file identified by
`uri`, so it is
+ // meaningless without `uri`. A group that declares `offset` but not
`uri` can never produce a
+ // valid value, so reject it at schema-build time.
+ Preconditions.checkArgument(
+ !hasOffset || hasUri,
+ "FILE type group '%s' declares field 'offset' but not 'uri';
'offset' locates a range "
+ + "within an external file and may only be set together with
'uri'",
+ name);
+ // The spec requires `size` to be set whenever `offset` is set. A group
that declares
+ // `offset` but not `size` can never produce a valid value, so reject it
at schema-build
+ // time.
+ Preconditions.checkArgument(
+ !hasOffset || hasSize,
+ "FILE type group '%s' declares field 'offset' but not 'size'; 'size'
is required whenever 'offset' is set",
Review Comment:
Same here, if all `offset` values are `null`, it is technically not set per
the spec and we can't assert on this here.
##########
parquet-column/src/main/java/org/apache/parquet/schema/Types.java:
##########
@@ -847,12 +847,115 @@ 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 hasUri = false;
+ boolean hasOffset = false;
+ boolean hasSize = false;
+ boolean hasInline = false;
+ for (Type field : fields) {
+ String fieldName = field.getName();
+ if
(!LogicalTypeAnnotation.FileLogicalTypeAnnotation.FIELD_NAMES.contains(fieldName))
{
+ throw new IllegalArgumentException("FILE type group '" + name + "'
contains unrecognized field '"
+ + fieldName + "'. Valid fields are: "
+ + String.join(", ",
LogicalTypeAnnotation.FileLogicalTypeAnnotation.FIELD_NAMES));
+ }
+ Preconditions.checkArgument(
+ field.isPrimitive() && field.getRepetition() ==
Type.Repetition.OPTIONAL,
+ "FILE type field '%s' must be an optional primitive in group '%s'",
+ fieldName,
+ name);
+ validateFileTypeFieldPhysicalType(name, field.asPrimitiveType());
+ if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.URI_FIELD.equals(fieldName)) {
+ hasUri = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.OFFSET_FIELD.equals(fieldName))
{
+ hasOffset = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.SIZE_FIELD.equals(fieldName)) {
+ hasSize = true;
+ } else if
(LogicalTypeAnnotation.FileLogicalTypeAnnotation.INLINE_FIELD.equals(fieldName))
{
+ hasInline = true;
+ }
+ }
+ // A value resolves to bytes only via inline storage or an external
reference, so a group must
+ // declare at least one of `inline` or `uri`. A group declaring neither
— even if it declares
+ // `offset` or `size` — can never produce a resolvable value, so reject
it at schema-build
+ // time.
+ Preconditions.checkArgument(
+ hasInline || hasUri,
+ "FILE type group '%s' must declare at least one of 'inline' or
'uri'; a value resolves to "
+ + "bytes only via inline storage or an external reference, so a
group declaring "
+ + "neither can never produce a valid value",
+ name);
+ // `offset` locates a byte range within the external file identified by
`uri`, so it is
+ // meaningless without `uri`. A group that declares `offset` but not
`uri` can never produce a
+ // valid value, so reject it at schema-build time.
+ Preconditions.checkArgument(
+ !hasOffset || hasUri,
+ "FILE type group '%s' declares field 'offset' but not 'uri';
'offset' locates a range "
+ + "within an external file and may only be set together with
'uri'",
+ name);
Review Comment:
I don't think we can make this assertion at the schema layer. If a FILE
group contains `inline` + `offset`, it could still be correct as long as all of
the values of the `offset` column are null. This is because the
[spec](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#file)
says:
> 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[1]). A field
is not set when it is absent from the group, or is present but null or empty.
--
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]
