Add support for date, time_millis, and timestamp_millis to AvroColumnDecoder #13070

tisonkun · 2022-07-03T13:28:59Z

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Avro part of trino-record-decoder.

How would you describe this change to a non-technical end user or system administrator?

Support parsing trino's time types in Avro column decoder.

Related issues, pull requests, and links

Fixes Add support for short timestamp, date and time to AvroColumnDecoder #13069.

From Avro's spec, actually all of these types should have either int or long in "type" field with a "logicalType" field distinguish.

So to support these types in AvroColumnDecoder aspect, simply add them to supported types list is enough and the actual parsing logic will be handled in caller side for example PulsarAvroRowDecoderFactory.

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.

This is almost an internal change.

( ) Release notes entries required with the following suggested text:

cla-bot · 2022-07-03T13:29:04Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

tisonkun · 2022-07-03T13:33:02Z

I have submitted CLA to [email protected]. Please check.

tisonkun · 2022-07-08T01:44:28Z

cc @kokosing I think I've sent the CLA already but the test still failed. Did I miss something here?

ebyhr · 2022-07-08T06:03:09Z

@tisonkun The registering process takes a few days. We will ping cla-bot once it's registered.

tisonkun · 2022-07-19T00:42:58Z

@ebyhr two weeks passed. May I have an estimate about your team's reviewing?

tisonkun · 2022-07-20T23:44:31Z

When I working on apache/pulsar#16683, I notice that after Trino supports parametric timestamp, the decode case should be review in another round.

Related to:

ebyhr · 2022-07-21T08:09:59Z

@cla-bot check

cla-bot · 2022-07-21T08:10:01Z

The cla-bot has been summoned, and re-checked this pull request!

lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java

lib/trino-record-decoder/src/test/java/io/trino/decoder/avro/TestAvroDecoder.java

tisonkun · 2022-07-25T03:57:31Z

ping @ebyhr @Praveen2112 @hashhar

hashhar

Why do we opt for custom encoding instead of using what Avro already provides out of the box?

See for example https://avro.apache.org/docs/current/spec.html#Timestamp+%28millisecond+precision%29

This custom encoding won't be readable by other tools which don't know how we've encoded the values.

tisonkun · 2022-07-29T06:14:32Z

@hashhar I don't get your point. This patch follow the Avro spec and adapt to Trino internal representation design.

Trino's TimestampType always store the payload in epochMicros precision.

tisonkun · 2022-07-29T06:16:03Z

And yes, it breaks downstream adoption as in apache/pulsar@c418395.

This is introduced by how we implement parametric timestamp type 21e3ddf, and out of the scope of this patch.

hashhar · 2022-07-29T06:28:39Z

But for other Avro readers to be able to get a timestamp value without manually decoding it the Avro schema needs to annotate the field with a logical type.

tisonkun · 2022-07-29T06:33:26Z

@hashhar You can read this commit 52f4cf2 that I revert trino-kafka adapting Avro logical type.

Still I don't know what changes you proposed here. It follows Avro spec, schema factory can infer Trino's type from Avro type and logical type, but that's the responsibility of downstream plugins.

lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java

hashhar · 2022-07-29T07:30:44Z

I'm a bit confused about this now. I probably need to re-read existing code more thoroughly and re-review later. In meantime I'll defer to @Praveen2112 .

tisonkun · 2022-08-09T09:57:52Z

ping @ebyhr @hashhar @Praveen2112

Praveen2112

Can we squash all the commits into one.

lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java

lib/trino-record-decoder/src/test/java/io/trino/decoder/avro/TestAvroDecoder.java

…Decoder squash. handle exactly time_millis and timestamp_millis type squash. test avro decoder squash. test avro schema converter squash. handle DateType when serialize record squash. fix style and revert trino-kafka changes squash. revert all trino-kafka changes squash. Update lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java squash. pass logical type in tests Signed-off-by: tison <[email protected]>

tisonkun · 2022-09-01T09:28:00Z

@Praveen2112 thanks for your reivews! Updated and squashed.

Praveen2112 · 2022-09-08T09:16:12Z

lib/trino-record-decoder/src/test/java/io/trino/decoder/avro/TestAvroDecoder.java

+    public void testIntDecodedAsTimeMillis()
+    {
+        DecoderTestColumnHandle row = new DecoderTestColumnHandle(0, "row", TIME_MILLIS, "id", null, null, false, false, false);
+        Map<DecoderColumnHandle, FieldValueProvider> decodedRow = buildAndDecodeColumn(row, "id", "{\"type\":\"int\",\"logicalType\":\"time-millis\"}", 100);


How about for time-micros ? I guess it is also an valid Avro logical type ? Similarly for timestamp

They can work. For the original purpose, I prepare this patch only to adapt Pulsar Trino Plugin's use case. Since the review process goes so long now, I'd prefer after a good result on the mills part and I may continue the micros part. Otherwise, it's extending the scope and I don't see an end.

BTW, Trino itself handles layout changes of TimestampType and TimeType in different PRs, so I think it's not a requirement we must do it at once.

I do agree we can have different PRs for a dedicated datatype. But is there a way to differentiate between if the data type is of time-micros or time-mills. If a datatype is annotated with a LogicalType time-micro how do we restrict decoding that datatype ?

It's the convertor's responsibility to pass the correct Trino type. AvroColumnDecoder accepts the Trino type the caller passes to it.

Praveen2112 · 2022-09-08T13:09:44Z

lib/trino-record-decoder/src/test/java/io/trino/decoder/avro/TestAvroDecoder.java

+    public void testLongDecodedAsTimestampMillis()
+    {
+        DecoderTestColumnHandle row = new DecoderTestColumnHandle(0, "row", TIMESTAMP_MILLIS, "id", null, null, false, false, false);
+        Map<DecoderColumnHandle, FieldValueProvider> decodedRow = buildAndDecodeColumn(row, "id", "{\"type\":\"long\",\"logicalType\":\"timestamp-millis\"}", 1658463479L);


I'm not sure if it is the correct way to test it. Since we pass the value to be serialized as long - Avro serializes/de-serializes it as long but as per specification it looks like for Java we would pass the input as Instant for Timestamp logical type - register the conversion for GeneircRecord and serialize the data so when we de-serialize we would get them as Instant instead of long (we could do the conversion manually). Similar mapping/conversion is implemented for Java classes generated by Avro.

GenericData.get().addLogicalTypeConversion(new TimeConversions.TimestampMillisConversion());

TypeConversion can be added using similar snippets.

Avro serializes/de-serializes it as long but as per specification it looks like for Java we would pass the input as Instant for Timestamp logical type

Yes. To support many and many cases, there's a lot of work to do. I don't think one PR should handle all of it or you can try it out. I'd say the patch here can resolve the specific issue of bumping trino version for Pulsar Trino Plugin.

GenericData.get().addLogicalTypeConversion(new TimeConversions.TimestampMillisConversion());

What does it mean? If I get it right you suggest one more possible case avro timestamp can be represented as. Then my answer is as above.

Yeah I agree but I'm afraid if this is the correct approach. One major thing which bugs me is that we manually do the conversion from a long to a micro-second precision compatible format - but ideally it should happen as a part of Avro library and it does support it. And one additional benefit is that in case if Avro modifies their internal representation it shouldn't affect us.

but ideally it should happen as a part of Avro library and it does support it

Can you show me how Avro "does support it"? I'm glad to adapt such a way.

This method shows on how Avro implements it
https://github.com/apache/avro/blob/a2c111dc3581f9549b173f7c6a9073cfbf173558/lang/java/avro/src/test/java/org/apache/avro/generic/TestGenericLogicalTypes.java#L368

Thank you! I'm looking into it this week :)

@Praveen2112 after a review I think the conversion is inevitable because Trino has it's own representation, introduced by:

21e3ddf

19811d3

I take a look at the TimeConvertions, but it seems to be used on the fly data in-memory processing, not during deserialization. Pulsar Trino Plugin users meet a use case that the actual payload is int/long according to the logical type, and there's no Instant or LocalTime to be converted.

I don't have a good idea to make further revision on this codepath. You're welcome to show a patch.

Any update on this patch?

cc @Praveen2112 @kokosing @hashhar

tisonkun · 2022-09-19T13:39:23Z

@Praveen2112

but ideally it should happen as a part of Avro library and it does support it.

You can directly send a patch onto this one for your suggestions, or at least point out what APIs provided by Avro are related to implementing it.

mosabua · 2024-01-12T22:53:01Z

👋 @tisonkun @Praveen2112 @hashhar - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

tisonkun · 2024-02-26T11:26:49Z

No longer work on this.

cla-bot bot added the cla-signed label Jul 21, 2022

ebyhr reviewed Jul 21, 2022

View reviewed changes

lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java Outdated Show resolved Hide resolved

lib/trino-record-decoder/src/test/java/io/trino/decoder/avro/TestAvroDecoder.java Outdated Show resolved Hide resolved

tisonkun requested a review from ebyhr July 22, 2022 04:38

ebyhr requested review from Praveen2112 and hashhar July 22, 2022 07:38

hashhar reviewed Jul 29, 2022

View reviewed changes

liul85 reviewed Jul 29, 2022

View reviewed changes

lib/trino-record-decoder/src/main/java/io/trino/decoder/avro/AvroColumnDecoder.java Outdated Show resolved Hide resolved

Praveen2112 reviewed Aug 26, 2022

View reviewed changes

tisonkun force-pushed the issue-13069 branch from a5d7dbe to 54dab35 Compare September 1, 2022 09:27

Praveen2112 reviewed Sep 8, 2022

View reviewed changes

hashhar requested a review from Praveen2112 October 13, 2022 08:14

tisonkun mentioned this pull request Oct 27, 2022

[Connector]Add plugin for Apache Pulsar. #8020

Closed

ebyhr removed their request for review February 22, 2023 07:11

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

tisonkun closed this Feb 26, 2024

tisonkun deleted the issue-13069 branch February 26, 2024 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for date, time_millis, and timestamp_millis to AvroColumnDecoder #13070

Add support for date, time_millis, and timestamp_millis to AvroColumnDecoder #13070

tisonkun commented Jul 3, 2022 •

edited

Loading

cla-bot bot commented Jul 3, 2022

tisonkun commented Jul 3, 2022

tisonkun commented Jul 8, 2022

ebyhr commented Jul 8, 2022

tisonkun commented Jul 19, 2022

tisonkun commented Jul 20, 2022

ebyhr commented Jul 21, 2022

cla-bot bot commented Jul 21, 2022

tisonkun commented Jul 25, 2022

hashhar left a comment

tisonkun commented Jul 29, 2022

tisonkun commented Jul 29, 2022

hashhar commented Jul 29, 2022

tisonkun commented Jul 29, 2022 •

edited

Loading

hashhar commented Jul 29, 2022

tisonkun commented Aug 9, 2022

Praveen2112 left a comment

tisonkun commented Sep 1, 2022

Praveen2112 Sep 8, 2022

tisonkun Sep 10, 2022

Praveen2112 Sep 12, 2022

tisonkun Sep 15, 2022

Praveen2112 Sep 8, 2022

Praveen2112 Sep 8, 2022

tisonkun Sep 10, 2022

Praveen2112 Sep 12, 2022

tisonkun Sep 15, 2022

Praveen2112 Sep 19, 2022

tisonkun Sep 28, 2022

tisonkun Sep 28, 2022 •

edited

Loading

tisonkun Apr 5, 2023

tisonkun commented Sep 19, 2022

mosabua commented Jan 12, 2024

tisonkun commented Feb 26, 2024

Add support for date, time_millis, and timestamp_millis to AvroColumnDecoder #13070

Add support for date, time_millis, and timestamp_millis to AvroColumnDecoder #13070

Conversation

tisonkun commented Jul 3, 2022 • edited Loading

Related issues, pull requests, and links

Documentation

Release notes

cla-bot bot commented Jul 3, 2022

tisonkun commented Jul 3, 2022

tisonkun commented Jul 8, 2022

ebyhr commented Jul 8, 2022

tisonkun commented Jul 19, 2022

tisonkun commented Jul 20, 2022

ebyhr commented Jul 21, 2022

cla-bot bot commented Jul 21, 2022

tisonkun commented Jul 25, 2022

hashhar left a comment

Choose a reason for hiding this comment

tisonkun commented Jul 29, 2022

tisonkun commented Jul 29, 2022

hashhar commented Jul 29, 2022

tisonkun commented Jul 29, 2022 • edited Loading

hashhar commented Jul 29, 2022

tisonkun commented Aug 9, 2022

Praveen2112 left a comment

Choose a reason for hiding this comment

tisonkun commented Sep 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Sep 19, 2022

mosabua commented Jan 12, 2024

tisonkun commented Feb 26, 2024

tisonkun commented Jul 3, 2022 •

edited

Loading

tisonkun commented Jul 29, 2022 •

edited

Loading

tisonkun Sep 28, 2022 •

edited

Loading