Skip to content

Conversation

@rdblue
Copy link
Contributor

@rdblue rdblue commented Mar 4, 2025

This updates Avro to support nanosecond timestamps and unknown.

This also cleans up some of the tests:

  • Moves DataTest to core so it can be used for Avro, rather than duplicating in AvroDataTest
  • Enables default value tests from DataTest for the internal object model (TestInternalAvro)
  • To use DataTest for TestInternalAvro, uses Record instead of StructLike and updates validations in InternalTestHelpers

The new tests in DataTest are currently disabled for Parquet, ORC, and Flink that reuse the DataTest base using supportsUnknown, supportsTimestampNanos, and supportsVariant. This allows breaking up the changes to support these types across multiple PRs.

@rdblue rdblue force-pushed the avro-readers-writers branch from 3144de8 to 39ba368 Compare March 4, 2025 23:46
Copy link
Contributor

@danielcweeks danielcweeks left a comment

Choose a reason for hiding this comment

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

+1 This all looks good to me. The only thing I note is that we're changing the terminology a little (opiton -> optional). While I agree with the using optional as it is less confusing, we're slightly inconsistent.

@rdblue
Copy link
Contributor Author

rdblue commented Mar 4, 2025

The only thing I note is that we're changing the terminology a little (option -> optional). While I agree with the using optional as it is less confusing, we're slightly inconsistent.

The distinction I was going for was between an optional field (may be null) and an option schema (a union of null and another type). I know it's a bit weird, but I didn't want to rename all of the uses of the latter meaning because we have isOptionSchema, toOption, and fromOption methods. I think this is the cleanest option.

@rdblue rdblue merged commit 3c8366a into apache:main Mar 5, 2025
43 checks passed
@rdblue
Copy link
Contributor Author

rdblue commented Mar 5, 2025

Thanks for reviewing, @danielcweeks!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants