Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

allow read old parquet file which is maked by old api with old avro version which allow wrong default value in schema #1140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

allow read old parquet file which is maked by old api with old avro version which allow wrong default value in schema #1140

wants to merge 1 commit into from

Conversation

wwang-talend
Copy link

@wwang-talend wwang-talend commented Sep 14, 2023 •
edited
Loading

Please consider if user call this api to write a parquet file with avro schema string in file metadata:

ParquetWriter<GenericRecord> writer = AvroParquetWriter
                    .<GenericRecord> builder(tempFile)
                    .withWriteMode(ParquetFileWriter.Mode.OVERWRITE)
                    .withSchema(avroSchema)
                    .build();

that "avroSchema" have wrong default value(as before avro 1.9, avro api not validate default value when construct avro schema).

Now user use new parquet-avro jar with avro 1.11.x, and do read action to read that old parquet file with avro schema string in file metadata, then will check the default value by parse avro schema in parquet AvroReadSupport, then issue will appear. That is why do this change here.

exception stack is this:

(org.apache.avro.AvroTypeException) Invalid default for field id: "" not a ["int","null"]
	at org.apache.avro.Schema.validateDefault(Schema.java:1636) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.avro.Schema.access$500(Schema.java:94) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.avro.Schema$Field.<init>(Schema.java:561) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.avro.Schema.parse(Schema.java:1747) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.avro.Schema$Parser.parse(Schema.java:1472) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.avro.Schema$Parser.parse(Schema.java:1459) ~[avro-1.11.2.jar:1.11.2]
	at org.apache.parquet.avro.AvroReadSupport.prepareForRead(AvroReadSupport.java:124) ~[parquet-avro-1.10.1.jar:1.10.1]
	at org.apache.parquet.hadoop.InternalParquetRecordReader.initialize(InternalParquetRecordReader.java:183) ~[parquet-hadoop-1.10.1.jar:1.10.1]
	at org.apache.parquet.hadoop.ParquetReader.initReader(ParquetReader.java:156) ~[parquet-hadoop-1.10.1.jar:1.10.1]
	at org.apache.parquet.hadoop.ParquetReader.read(ParquetReader.java:135) ~[parquet-hadoop-1.10.1.jar:1.10.1]

@wwang-talend
allow read old parquet file which is maked by old api with old avro v…
dd6bd57 
…ersion which allow wrong default value in schema
@wgtmac
Copy link
Member

wgtmac commented Sep 20, 2023

Thanks @wwang-talend for opening the PR! Could you create a JIRA issue for this?

wgtmac
wgtmac reviewed Sep 20, 2023
View reviewed changes
parquet-avro/src/main/java/org/apache/parquet/avro/AvroReadSupport.java
@@ -129,10 +129,10 @@ public RecordMaterializer<T> prepareForRead(
avroSchema = new Schema.Parser().parse(metadata.get(AVRO_READ_SCHEMA_METADATA_KEY));
} else if (keyValueMetaData.get(AVRO_SCHEMA_METADATA_KEY) != null) {
// use the Avro schema from the file metadata if present
avroSchema = new Schema.Parser().parse(keyValueMetaData.get(AVRO_SCHEMA_METADATA_KEY));
avroSchema = new Schema.Parser().setValidateDefaults(false).parse(keyValueMetaData.get(AVRO_SCHEMA_METADATA_KEY));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this issue exist with both AVRO_SCHEMA_METADATA_KEY and OLD_AVRO_SCHEMA_METADATA_KEY?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blindly disabling validation at all times may introduce new issues silently. Is it safer to add a flag to work around this? In the default case, we should always enable validation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc Avro experts @Fokko @gszadovszky @clairemcginty

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does seem potentially scary - maybe it would be safest to make this a Configuration option that the user can opt into?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 with option to enable this or not

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 making it a config option

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fg-mapp fg-mapp fg-mapp left review comments

@ConeyLiu ConeyLiu ConeyLiu left review comments

@wgtmac wgtmac wgtmac left review comments

@clairemcginty clairemcginty clairemcginty left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wwang-talend @wgtmac @clairemcginty @ConeyLiu @fg-mapp