AVRO-4242: [Java] Fix NPE in DataFileStream and DataFileReader when schema metadata is missing - #3726
Conversation
iemejia
changed the title
3adb3a0 to
893d488
Compare
There was a problem hiding this comment.
Pull request overview
Fixes crashes when reading malformed Avro container files that omit schema metadata by introducing null-safe schema parsing that fails with a descriptive IOException instead of an NPE.
Changes:
- Replace inline schema parsing in
DataFileStreamwith a null-safe helper that throws descriptiveIOExceptions for missing/invalid schema metadata. - Reuse the same helper in
DataFileReader12to avoid NPEs when the 1.2-formatschemametadata is absent. - Add a regression test that builds a malformed container header (missing
avro.schema) and assertsDataFileStreamandDataFileReaderfail with a helpful message.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lang/java/avro/src/main/java/org/apache/avro/file/DataFileStream.java | Adds null-safe schema parsing from metadata and uses it during header initialization. |
| lang/java/avro/src/main/java/org/apache/avro/file/DataFileReader12.java | Uses the new helper to avoid NPE when schema metadata is missing in 1.2 format. |
| lang/java/avro/src/test/java/org/apache/avro/TestDataFileReader.java | Adds a regression test building a malformed header missing schema metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| private Schema parseSchema() throws IOException { | ||
| return DataFileStream.parseSchemaFromMetadata(getMetaString(SCHEMA), SCHEMA, new Schema.Parser()); |
|
@copilot do the improvements requested in the review |
| assertNotNull(streamException.getMessage()); | ||
| assertTrue(streamException.getMessage().contains(DataFileConstants.SCHEMA)); | ||
|
|
||
| IOException readerException = assertThrows(IOException.class, |
There was a problem hiding this comment.
You mentioned DataFileReader12 in the PR (and the problem was indeed fixed there too), does this test case give sufficient coverage to verify those changes too?
There was a problem hiding this comment.
Good point - the existing test only covers DataFileStream and DataFileReader (current format). It does not exercise DataFileReader12 since that class uses a different binary layout (footer-based metadata).
I've now added a dedicated test (missingSchemaMetadataInVersion12DoesNotThrowNullPointerException) that constructs a minimal Avro 1.2 format container with the sync marker but no schema entry, and asserts that DataFileReader12 throws a descriptive IOException rather than an NPE.
ca5ad50 to
eaec9e1
Compare
…chema metadata is missing Malformed Avro container files without the 'avro.schema' metadata entry caused a NullPointerException in both DataFileStream and DataFileReader12 when the null value was passed directly to Schema.Parser.parse(). Replace inline parsing with null-safe helper methods that throw a descriptive IOException instead.
eaec9e1 to
7b7c712
Compare
|
Cherry picked for 1.12 at #3844 |
Malformed Avro container files without the 'avro.schema' metadata entry caused a NullPointerException in both DataFileStream and DataFileReader12 when the null value was passed directly to Schema.Parser.parse(). Replace inline parsing with null-safe helper methods that throw a descriptive IOException instead.
R: @RyanSkraba