Native Avro File Reader

[jklamer]

Description

Add classes and utilities to replace Hive library AvroSerde deserializing functionality by creating a Trino native Avro page source.

The changes are largely split into two module:

• trino-hive-formats General use avro library that will allow connectors to define custom page building code or use default behavior. Responsible for resolution of read/write schemas and file bytes decoding.
• trino-hive The Hive plugins usage of the above library to ensure backwards compatibility with current implementation along with custom schema sourcing code. Includes custom solutions for handling reader projection use cases.

Main features:

• Column masking pushdown to decrease memory footprint for most queries
• Reading bytes directly from file into pages without Object creation or casting in majority of cases

Main classes for review:

• io.trino.hive.formats.avro.AvroPageDataReader:
  • A cross between org.apache.avro.io.FastReaderBuilder and io.trino.hive.formats.line.json.JsonDeserializer
  • impelents org.apache.avro.io.DatumReader
  • Accumulates pages as the Avro Library reads through the file. Handling schema read resolution (reordering, skips, defaults, promotions)
• io.trino.plugin.hive.avro.AvroHiveFileUtils: handles HiveType -> Avro Schema mapping in place of org.apache.hadoop.hive.serde2.avro.AvroSerDe
• io.trino.plugin.hive.avro.HiveAvroTypeManager: customizes the format library with special HiveType and Timestamp transformations in a backwards compatible way. Flattens behavior currently located in a number of files into functions it supplies to the library.

Additional context and related issues

This is part of a broader effort to optionalize the Trino dependency on hive/hadoop.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(X) Release notes are required, with the following suggested text:

New Avro Native Reader enabled by default in trine-hive plugin.
Disable with config avro.native-reader.enabled=false or using session property SET SESSION <catalog_name>.avro_native_reader_enabled = false if experiencing issues with this new reader.

[dain]

I'm still reviewing but here are some initial comments

[dain]

You should to use the TypeOperators from ConnectorContext.getTypeManager().getTypeOperators(), but if that is too difficult you could create a static final. The TypeOperators are effectiely generating code with method handles so if you are constantly creating new instances, the code will always be cold (a.k.a., slow)

[jklamer]

I might need some help wiring this up the way you're envisioning

[dain]

reminder we need to resolve this one

[dain]

Do we really need this dependency? I would prefer if we did not need this in this module.

[jklamer]

Im using it for io.trino.block.BlockAssertions can I move that class to SPI?

[jklamer]

should I make own version?

[electrum]

I think we should move it to the SPI. When you do that, switch the TestNG assertEquals usage to AssertJ, since we're trying to move away from TestNG in new code.

[jklamer]

Getting the rolling IntelliJ errors on the refactor. I'll ping you offline on what you think we should do here.

Field ColorType.COLOR, referenced in method BlockAssertions.createColorRepeatBlock(int, int), will not be accessible in module trino-spi  Field TestingConnectorSession.SESSION, referenced in method BlockAssertions.getOnlyValue(Type, Block), will not be accessible in module trino-spi  Field IpAddressType.IPADDRESS, referenced in method BlockAssertions.createRandomBlockForType(Type, int, float), will not be accessible in module trino-spi

[dain]

I reviewed Add Native Avro to Page code with connector defined mappings commit. My comments are mostly stylistic, and the code looks good.

I am concerned about the testing. Most of the tests in this commit are just verifying the correct number of rows and that no exceptions were thrown. I think we need basic tests for every type and a few complex combinations of types.

[dain]

this is really nice

[jklamer]

Check

[rdblue]

I probably wouldn't recommend using the resolver. The only thing that needs to be resolved is the order to read struct fields, skip, and then how to fill in defaults. That's actually pretty simple and can be done directly. Check out our implementation in Python.

Basically, that creates a list of (optional position, reader) pairs by doing the following:

1. Loop over the file schema's record and create a pair for every field
   • If there is a corresponding field in the read schema, use that field's position from the read schema
   • If the field is not in the read schema, use empty
2. Loop over the read schema and add any needed pairs
   • If the field was in the file schema, skip it
   • If the field was missing, create a pair from the field's position in the read schema and a constant reader with the field's default

Then to read, you just loop over those pairs. If the position is present, get the value from the reader and set that position in the record. If the position is not present, call skip to consume and discard the bytes.

I think this is overall way cleaner, although I like what you're doing here and using the resolver to create your reader tree, rather than resolving in each record.

[jklamer]

Oh that's a good point. I definitely don't love the data model of the Actions returned for this or other use cases. When I was implementing I was just kinda peeling back layers of Generic Datum Reader and FastDatumReader until I didn't need to anymore and just pretty much copied the use from those classes. The reason I might keep using it is that is also provides Reader and Writer Union resolution steps that I originally wan't super confident on implementing but need (I imagine I could but would need to think on it, I think it would involve load bearing exceptions). From my understanding after looking at the pyiceberg code, Iceberg doesn't support non-trivial(optional) unions and doesn't need to do union resolution correct?

[electrum]

Do you want to address this now or in a follow up?

[jklamer]

I'll ping @rdblue offline and we'll get this sorted.

[electrum]

Capitalize the commit titles: https://cbea.ms/git-commit/

[electrum]

Do you want to address this now or in a follow up?

[electrum]

What would happen if we encounter a file with duplicate keys?

[electrum]

Is this something we plan to support? I'm wondering how the log message might be useful.

[jklamer]

I would actually like to talk with you about this. I want to make sure I understand our TimestampWithTimeZoneType enough to know if maps directly to this logical type or can be coerced in a way that makes sense.

[jklamer]

@b-slim @weijiii , be aware this has been merged. Let me know if you find any issues with it while using.