Skip to content

Conversation

@Tishj
Copy link

@Tishj Tishj commented Jun 17, 2025

This PR makes it so that a different serialization of the null type is accepted, this variation is produced by avro-c (apache/avro's c lang implementation)

Problem described in duckdb/duckdb-iceberg#305 (comment)

Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

this makes sense, but i cant find a corresponding mention in the avro spec

cc @Fokko to chime in here

@Tishj
Copy link
Author

Tishj commented Jun 17, 2025

The spec seems to indicate that this is valid:
https://avro.apache.org/docs/1.11.1/specification/

Right under Schema Declaration

{"type": "typeName" ...attributes...}

where typeName is either a primitive or derived type name, as defined below

List of primitive types

Primitive Types
The set of primitive type names is:

null: no value
...

And then below the list:

Primitive type names are also defined type names. Thus, for example, the schema “string” is equivalent to:
{"type": "string"}

Which is why "null" is shorthand for {"type": "null"}

@kevinjqliu
Copy link
Contributor

ty that makes sense, thanks for the pointer

@kevinjqliu
Copy link
Contributor

Looks like theres a linter issue, could you run make lint locally?

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

While the syntax looks a bit weird, I agree that this is valid:

image

I agree with @kevinjqliu that a test would be a very good idea, since we're moving this part to rust somewhere in the not-so-distant future :)

@kevinjqliu kevinjqliu requested a review from Fokko June 21, 2025 17:29
@kevinjqliu
Copy link
Contributor

@Tishj I was not able to find or generate an avro manifest list file to verify this. Do you have one?

@Tishj Tishj closed this Jul 6, 2025
@kevinjqliu
Copy link
Contributor

@Tishj is this fix not needed anymore?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants