Make sealed version of Avro Resolver Action #21407
Conversation
jklamer
force-pushed
the
jklamer/AvroCompilationAsSealed
branch
from
3feca8c to
a87c9fb
Compare
|
@jklamer what's that, and where are the tests for it? :) |
|
@wendigo It's an internal Avro model that can be improved via sealed. The plan is to expose it to plugin writers so making it sealed is for ergonomics. The tests that currently exist should suffice as this PR does not change logic, but just location and structure of currently class internal structures. |
lib/trino-hive-formats/src/main/java/io/trino/hive/formats/avro/BlockBuildingDecoder.java
Outdated
Show resolved
Hide resolved
lib/trino-hive-formats/src/main/java/io/trino/hive/formats/avro/model/BoolRead.java
Outdated
Show resolved
Hide resolved
...trino-hive-formats/src/main/java/io/trino/hive/formats/avro/model/RecordFieldReadAction.java
Outdated
Show resolved
Hide resolved
...trino-hive-formats/src/main/java/io/trino/hive/formats/avro/model/RecordFieldReadAction.java
Outdated
Show resolved
Hide resolved
| private final Schema readSchema; | ||
| private final Schema writeSchema; |
There was a problem hiding this comment.
It looks like every implementation has these two fields, which are check for null and assigned in the constructor of every implementation. How about we move these to the base class.
There was a problem hiding this comment.
Alternatively, I think every implementation of AvroReadAction and RecordFieldReadAction can be a simple record (with a constructor to verify arguments), so I'd do that instead.
There was a problem hiding this comment.
Most implementations can be simple records. For types that can be promoted to, another field is present to hold the promotion function (eg int -> long)
There was a problem hiding this comment.
I'll make everything records I can and see how we feel
There was a problem hiding this comment.
oh I don't have to manually implement the methods for the interface when they're records. Thats aggressively slick. Doing that as much as possible
...e-formats/src/main/java/io/trino/hive/formats/avro/model/SkipFieldRecordFieldReadAction.java
Outdated
Show resolved
Hide resolved
| }; | ||
| } | ||
|
|
||
| @FunctionalInterface | ||
| private interface LongIoFunction<A> | ||
| public interface LongIoFunction<A> |
There was a problem hiding this comment.
Consider moving these to the classes like LongRead (can be follow up)
There was a problem hiding this comment.
Need to do a lot of shuffling when rewriting these pieces. Will happen in follow up
lib/trino-hive-formats/src/main/java/io/trino/hive/formats/avro/model/EnumReadAction.java
Outdated
Show resolved
Hide resolved
...rino-hive-formats/src/main/java/io/trino/hive/formats/avro/model/WrittenUnionReadAction.java
Outdated
Show resolved
Hide resolved
...s/src/main/java/io/trino/hive/formats/avro/model/DefaultValueFieldRecordFieldReadAction.java
Outdated
Show resolved
Hide resolved
jklamer
force-pushed
the
jklamer/AvroCompilationAsSealed
branch
from
f17dafb to
d5239d8
Compare
jklamer
force-pushed
the
jklamer/AvroCompilationAsSealed
branch
from
d5239d8 to
4329fab
Compare
Description
Make a sealed version of the Avro Resolver Action
Additional context and related issues
Release notes
( X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: