You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "aihuaxu (via GitHub)" <gi...@apache.org> on 2025/07/27 20:36:09 UTC
[PR] Test Variant read from files [parquet-java]
aihuaxu opened a new pull request, #3258:
URL: https://github.com/apache/parquet-java/pull/3258
<!--
Thanks for opening a pull request!
If you're new to Parquet-Java, information on how to contribute can be found here: https://parquet.apache.org/docs/contribution-guidelines/contributing
Please open a GitHub issue for this pull request: https://github.com/apache/parquet-java/issues/new/choose
and format pull request title as below:
GH-${GITHUB_ISSUE_ID}: ${SUMMARY}
or simply use the title below if it is a minor issue:
MINOR: ${SUMMARY}
-->
### Rationale for this change
This is to test out variant implementation in Parquet java against the test cases generated in https://github.com/apache/parquet-testing/pull/90/files.
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
<!-- Please uncomment the line below and replace ${GITHUB_ISSUE_ID} with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases (not ready for merge) [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2241357500
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
I think we need this change that we will produce a fixed schema of value and metadata rather than reading the original avro schema.
Also, we also fix the issue that value field may be missing from the schema since that is allowed, i.e., typed_value exists but value doesn't. We don't read value field for the position and the output schema should be fixed.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2258299822
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
@cashmand I followed up with Ryan and I got more context around Avro.
For some reader path, seems we are passing AvroSchema as a shredded schema which should not happen. Let me checkout more how that happened. The AvroSchema here should always be a record of (value, metadata).
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2240703580
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroRecordConverter.java:
##########
@@ -395,7 +395,7 @@ private static Converter newConverter(
}
return newStringConverter(schema, model, parent, validator);
case RECORD:
- if (type.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.VariantLogicalTypeAnnotation) {
+ if (type.getName().equals("var") || type.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.VariantLogicalTypeAnnotation) {
Review Comment:
I don't think that this PR can be merged until this is no longer required.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "cashmand (via GitHub)" <gi...@apache.org>.
cashmand commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2258022900
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
I think the intent was the parquet schema should be the shredding schema, but the Avro schema provided for Variant should always be a record of `(value, metadata)`, even if the parquet schema doesn't contain `value`. So as @rdblue, we should reject the schema if that isn't what was provided for a Variant column.
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
I think the intent was the parquet schema should be the shredding schema, but the Avro schema provided for Variant should always be a record of `(value, metadata)`, even if the parquet schema doesn't contain `value`. So as @rdblue said, we should reject the schema if that isn't what was provided for a Variant column.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #3258: Test Variant implementation with external test cases
URL: https://github.com/apache/parquet-java/pull/3258
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2248853529
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
We can't change the expected Avro schema. We can reject it, but the contract is to use the schema that was passed in. I think the earlier code is correct.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "cashmand (via GitHub)" <gi...@apache.org>.
cashmand commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2236421513
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
I'm not sure I understand the question. Are you saying that we should be setting the Variant object directly here, rather than a `(metadata, value)` record? That might be reasonable, my understanding of Avro, and how it's meant to be used, is pretty weak. cc @rdblue
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2237151049
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
That's right. Seems we should return Variant rather than `(metadata, value)` record.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases (not ready for merge) [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2241317311
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
By reading the mapping in https://iceberg.apache.org/spec/#avro, actually we are expecting a record of metadata and value in Avro for variant.
Looks like that we just need to update the test rather than making the code. @rdblue and @cashmand
<img width="693" height="136" alt="image" src="https://github.com/user-attachments/assets/83f3d7b6-0edf-498d-a45e-43779c224ad5" />
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2243493263
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java:
##########
@@ -470,11 +470,8 @@ public Optional<Schema> visit(
@Override
public Optional<Schema> visit(
LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
- String name = parquetGroupType.getName();
- List<Schema.Field> fields = new ArrayList<>();
- fields.add(new Schema.Field("metadata", Schema.create(Schema.Type.BYTES)));
- fields.add(new Schema.Field("value", Schema.create(Schema.Type.BYTES)));
- return of(Schema.createRecord(name, null, namespace(name, names), false, fields));
+ return of(
Review Comment:
@rdblue and @cashmand Here is another issue that I have noticed:
If the messageType is as follows, do we generate the schema including everything like typed_value or we just include metadata and value? In Iceberg code, right now we are expecting full schema, e.g., we will set the value to `typed_value` field.
```
GroupType variantType =
Types.buildGroup(Type.Repetition.REQUIRED)
.as(LogicalTypeAnnotation.variantType((byte)1))
.id(2)
.required(PrimitiveTypeName.BINARY)
.named("metadata")
.addField(objectFields)
.named("var");
MessageType parquetSchema = parquetSchema(variantType);
GenericRecord recordA = record(fieldA, Map.of("value", serialize(Variants.of((short) 1234))));
GenericRecord recordB = record(fieldB, Map.of("typed_value", "iceberg"));
```
```
message table {
required int32 id;
optional group var (VARIANT(1)) {
required binary metadata;
optional binary value;
optional group typed_value {
required group a {
optional binary value;
optional group typed_value (LIST) {
repeated group list {
required group element {
optional binary value;
optional binary typed_value (STRING);
}
}
}
}
}
}
}
```
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2248852649
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -34,21 +35,27 @@
* Converter for Variant values.
*/
class AvroVariantConverter extends GroupConverter implements VariantConverters.ParentConverter<VariantBuilder> {
+ private static final Schema VARIANT_SCHEMA = SchemaBuilder.record("VariantRecord")
+ .fields()
+ .name("metadata")
+ .type()
+ .bytesType()
+ .noDefault()
+ .name("value")
+ .type()
+ .bytesType()
+ .noDefault()
+ .endRecord();
+
private final ParentValueContainer parent;
- private final Schema avroSchema;
private final GenericData model;
- private final int metadataPos;
- private final int valuePos;
Review Comment:
Why are all of these changes needed? I think this should continue to use the schema that was passed in rather than replacing it with a different one.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2237153379
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
I want to confirm with you and I will can make the change.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2237964711
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
Confirmed with @rdblue offline and it should return a variant. Let me work on the fix and also address write side as well.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#issuecomment-4526903368
Closing this pull request due to at least 3 months of inactivity. If you would like to continue the work, please feel free to reopen this pull request or open a new one. Thank you for your contribution to Apache Parquet!
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2234133629
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
@cashmand Would this convert to Variant object 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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2268635129
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroRecordConverter.java:
##########
@@ -396,7 +408,7 @@ private static Converter newConverter(
return newStringConverter(schema, model, parent, validator);
case RECORD:
if (type.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.VariantLogicalTypeAnnotation) {
- return new AvroVariantConverter(parent, type.asGroupType(), schema, model);
+ return new AvroVariantConverter(parent, type.asGroupType(), VARIANT_SCHEMA, model);
Review Comment:
We are reading avro schema from the `parquet.avro.schema` of parquet file . If this schema doesn't have value field, then in [AvroVariantConverter.java#L51](https://github.com/apache/parquet-java/blob/master/parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java#L51), it will error out.
The schema in `parquet.avro.schema` seems to be correct to represent the write schema and we we need to make the change to pass in record of value + metadata for variant logical type.
```
aixu@K7YJWY4PK6 variant % parquet2 meta case-111.parquet
File path: case-111.parquet
Created by: parquet-mr version 1.16.0-SNAPSHOT (build ee34713e4d906d61f95d2b09145945638b2e2296)
Properties:
parquet.avro.schema: {"type":"record","name":"table","fields":[{"name":"id","type":"int"},{"name":"var","type":["null",{"type":"record","name":"var","fields":[{"name":"metadata","type":"bytes"},{"name":"value","type":["null","bytes"],"default":null},{"name":"typed_value","type":["null","string"],"default":null}]}],"default":null}]}
writer.model.name: avro
Schema:
message table {
required int32 id = 1;
optional group var (VARIANT(1)) = 2 {
required binary metadata;
optional binary value;
optional binary typed_value (STRING);
}
}
```
Let me know if I misunderstand `parquet.avro.schema`.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2249033911
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,9 +84,9 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
+ Object record = model.newRecord(null, VARIANT_SCHEMA);
Review Comment:
Here what we are passing in the shredded schema which may/may not have `value` or `typed_value` field.
But we want to generate a variant which maps to a record of `value` and `metadata` fields, right?
<img width="693" height="136" alt="image" src="https://github.com/user-attachments/assets/007b8b31-ea8b-4c47-acc5-57533a1a3d99" />
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#issuecomment-4309479326
This pull request has been automatically marked as stale because it has had no activity for at least 2 months. If you are still working on this change or plan to move it forward, please leave a comment or push a new commit so we know to keep it open. Otherwise, this PR will be closed automatically in about one month. Thank you for your contribution to Apache Parquet!
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2248855094
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java:
##########
@@ -470,11 +470,8 @@ public Optional<Schema> visit(
@Override
public Optional<Schema> visit(
LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
- String name = parquetGroupType.getName();
- List<Schema.Field> fields = new ArrayList<>();
- fields.add(new Schema.Field("metadata", Schema.create(Schema.Type.BYTES)));
- fields.add(new Schema.Field("value", Schema.create(Schema.Type.BYTES)));
- return of(Schema.createRecord(name, null, namespace(name, names), false, fields));
+ return of(
Review Comment:
Is that the behavior in production code or test code?
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2249032710
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java:
##########
@@ -470,11 +470,8 @@ public Optional<Schema> visit(
@Override
public Optional<Schema> visit(
LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
- String name = parquetGroupType.getName();
- List<Schema.Field> fields = new ArrayList<>();
- fields.add(new Schema.Field("metadata", Schema.create(Schema.Type.BYTES)));
- fields.add(new Schema.Field("value", Schema.create(Schema.Type.BYTES)));
- return of(Schema.createRecord(name, null, namespace(name, names), false, fields));
+ return of(
Review Comment:
It's in the test code that we use AvroSchemaConverter to create the schema. With the current logic, if we pass in a shredded variant type like
```
GroupType variantType =
Types.buildGroup(Type.Repetition.REQUIRED)
.as(LogicalTypeAnnotation.variantType((byte)1))
.id(2)
.required(PrimitiveTypeName.BINARY)
.named("metadata")
.addField(objectFields)
.named("var");
```
We are converted to a record of `value` and `metadata`, then we cannot set the value for `typed_value`.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2249036054
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -34,21 +35,27 @@
* Converter for Variant values.
*/
class AvroVariantConverter extends GroupConverter implements VariantConverters.ParentConverter<VariantBuilder> {
+ private static final Schema VARIANT_SCHEMA = SchemaBuilder.record("VariantRecord")
+ .fields()
+ .name("metadata")
+ .type()
+ .bytesType()
+ .noDefault()
+ .name("value")
+ .type()
+ .bytesType()
+ .noDefault()
+ .endRecord();
+
private final ParentValueContainer parent;
- private final Schema avroSchema;
private final GenericData model;
- private final int metadataPos;
- private final int valuePos;
Review Comment:
We are passing in a schema which may not have `value` or `typed_value` fields. As I understand, we want to return a record of `value` and `metadata` fields for a variant in Avro.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2249032710
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroSchemaConverter.java:
##########
@@ -470,11 +470,8 @@ public Optional<Schema> visit(
@Override
public Optional<Schema> visit(
LogicalTypeAnnotation.VariantLogicalTypeAnnotation variantLogicalType) {
- String name = parquetGroupType.getName();
- List<Schema.Field> fields = new ArrayList<>();
- fields.add(new Schema.Field("metadata", Schema.create(Schema.Type.BYTES)));
- fields.add(new Schema.Field("value", Schema.create(Schema.Type.BYTES)));
- return of(Schema.createRecord(name, null, namespace(name, names), false, fields));
+ return of(
Review Comment:
It's in the test code that we use AvroSchemaConverter to create the schema in https://github.com/apache/iceberg/blob/main/parquet/src/test/java/org/apache/iceberg/parquet/TestVariantReaders.java#L1597. With the current logic, if we pass in a shredded variant type like
```
GroupType variantType =
Types.buildGroup(Type.Repetition.REQUIRED)
.as(LogicalTypeAnnotation.variantType((byte)1))
.id(2)
.required(PrimitiveTypeName.BINARY)
.named("metadata")
.addField(objectFields)
.named("var");
```
We are converted to a record of `value` and `metadata`, then we cannot set the value for `typed_value`.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2248855974
##########
parquet-avro/src/test/java/org/apache/parquet/variant/TestVariantReadsFromFile.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.variant;
+
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Streams;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.stream.Stream;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.avro.AvroParquetReader;
+import org.apache.parquet.io.InputFile;
+import org.apache.parquet.io.LocalInputFile;
+import org.assertj.core.api.Assertions;
Review Comment:
This uses JUnit 5. Does that work in Parquet?
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2279997054
##########
parquet-avro/src/test/java/org/apache/parquet/variant/TestVariantReadsFromFile.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.parquet.variant;
+
+import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
+
+import com.fasterxml.jackson.databind.JsonNode;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Streams;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.file.Paths;
+import java.util.List;
+import java.util.stream.Stream;
+import org.apache.avro.generic.GenericData;
+import org.apache.avro.generic.GenericRecord;
+import org.apache.parquet.Preconditions;
+import org.apache.parquet.avro.AvroParquetReader;
+import org.apache.parquet.io.InputFile;
+import org.apache.parquet.io.LocalInputFile;
+import org.assertj.core.api.Assertions;
Review Comment:
I have added the dependency in pom.xml and seems to be working.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases [parquet-java]
Posted by "cashmand (via GitHub)" <gi...@apache.org>.
cashmand commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2237240278
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroVariantConverter.java:
##########
@@ -77,10 +78,12 @@ public void end() {
builder.appendNullIfEmpty();
- Object record = model.newRecord(null, avroSchema);
- model.setField(record, "metadata", metadataPos, metadata.getEncodedBuffer());
- model.setField(record, "value", valuePos, builder.encodedValue());
- parent.add(record);
+ Variant variant = builder.build();
+ parent.add(variant);
Review Comment:
It sounds fine to me, I'd suggest getting @rdblue to approve. Maybe we want to do something similar on the write side, where I think it also currently expects a `metadata, value` pair from Avro rather than a Variant object.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] Test Variant implementation with external test cases (not ready for merge) [parquet-java]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #3258:
URL: https://github.com/apache/parquet-java/pull/3258#discussion_r2241025767
##########
parquet-avro/src/main/java/org/apache/parquet/avro/AvroRecordConverter.java:
##########
@@ -395,7 +395,7 @@ private static Converter newConverter(
}
return newStringConverter(schema, model, parent, validator);
case RECORD:
- if (type.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.VariantLogicalTypeAnnotation) {
+ if (type.getName().equals("var") || type.getLogicalTypeAnnotation() instanceof LogicalTypeAnnotation.VariantLogicalTypeAnnotation) {
Review Comment:
That's right. I workaround to test the compatibility for now.
--
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: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org