Add support for timestamp type to Pinot connector by ddcprg · Pull Request #12145 · trinodb/trino

ddcprg · 2022-04-26T22:31:20Z

Description

Add support for TIMESTAMP data type in Pinot connector. Help fixing #10199

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)
Pinot connector

How would you describe this change to a non-technical end user or system administrator?
Include additional Pinot data types in Pinot connector

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(X) 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

( ) No release notes entries required.
(X) Release notes entries required with the following suggested text:

# Section
* Add support for TIMESTAMP data type in Pinot connector. ({issue}`10199`)

ddcprg · 2022-04-26T22:35:36Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/conversion/PinotTimestamps.java

See https://github.com/apache/pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java#L468

ddcprg · 2022-04-26T22:37:13Z

plugin/trino-pinot/src/test/resources/alltypes_schema.json

I'll add one more test column for multi-value timestamps - I need to check this is actually supported

I'll get back to this once apache/pinot#8624 is resolved

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotColumnHandle.java

hashhar · 2022-04-27T06:07:05Z

@elonazoulay can you please help with an initial review? I won't be able to review this thoroughly until ~9th May so expect some delays.

xiangfu0 · 2022-04-27T22:32:11Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotColumnHandle.java

Should this be JsonType or VarcharType? How JSON functions will be handled here e.g. json_extract ?

I think JsonType is not part of the SPI https://github.com/trinodb/trino/tree/master/core/trino-spi/src/main/java/io/trino/spi/type and is a type only available to functions in query time, I'll double check that just in case I missed it. I was going to leave functions out of the scope of this PR but I'll check how easy it would be to include them otherwise I'll raise a separate issue for this. Probably the only 2 function that could be easily mapped are json_extract_scalar and json_format as each has an equivalent in Pinot. I'll get back to you on this.

I see, can you test it locally with PinotJsonQuickStart?
Here is a reference PR as well.
https://github.com/prestodb/presto/pull/17015/files#diff-379277f1690c545f2f182815aa2b984399e26318df7e2402be7149a2c5e035b4R135

prestodb/presto#17323

Thanks @xiangfu0 I'll take a look at those. In the meantime it seems I've found an issue with multi-value timestamp columns, I've raised an issue in the pinot project apache/pinot#8624 I'll leave the test code for timestamp arrays commented out and refer to this issue in a comment for the time being. I'll try to raise PR to fix it

You can get JsonType via TypeManager. PostgreSqlClient has the example.

I think it would be better to revert this change and implement in #13428

nizarhejazi · 2022-04-29T07:52:46Z

@elonazoulay can you please help with PR review?

nizarhejazi · 2022-05-06T19:57:56Z

@ddcprg thanks for putting this together. @hashhar @elonazoulay @xiangfu0 Can we do another round of reviews on Monday May 9th? Thanks.

nizarhejazi · 2022-05-10T18:38:46Z

@hashhar @elonazoulay @xiangfu0 Can you review PR and sign off if no blocker?

hashhar · 2022-05-11T05:26:06Z

I'll take a look during the end of this week.

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/conversion/PinotTimestamps.java

hashhar · 2022-05-15T05:41:30Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/decoders/TimestampDecoder.java

Avoid casts from Number. Byte, BigDecimal etc. are all subclasses of Number.

changed this to Long

@hashhar seems like the test fails because the aggregate function returns double instead of long, I need to dig further if we don't want to cast from Number

@hashhar @xiangfu0 Pinot's MAX function returns double https://docs.pinot.apache.org/users/user-guide-query/supported-aggregations I think the options are either keeping Number or having 2 conditionals: one for Double and one for Long. I rather keep checking for Number type, let me know your thoughts

I think number is ok.

having else if (value instanceof Double || value instanceof Long) is still useful in my opinion.

I've now made this change

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java

hashhar · 2022-05-15T05:45:35Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java

Does this return correct results if both a null and 1970-01-01T00:00:00 exist?

the default null value for timestamps is epoch so this should return 1970-01-01T00:00:00 https://docs.pinot.apache.org/configuration-reference/schema#dimensionfieldspec

Then I would argue it's unsafe to push-down aggregates in presence of nulls. Maybe we can instead use query-passthrough (by implementing a polymorphic table function as in #12325) for such cases to get Pinot semantics.

cc: @findepi Any opinions? i.e. DISTINCT on a column with null and epoch would return epoch since the null becomes epoch.

Does null get converted to epoch only when DISTINCT is applied?
If SELECT return null and epoch then SELECT DISTINCT should still do that, and SELECT count(DISTINCT ..) should return 2.

@hashhar worth adding correctness smoke tests for null vs 0 and null vs '' in BCT.

I'm going to double check and get back to you. I think this is the same behaviour of other types like long and double

yes, Pinot schema will assign the default null value. So this behavior is expected.

We shouldn't push down such queries then probably. While the results match semantics of Pinot they don't match Trino so anyone performing a JOIN or SELECT across two catalogs will see inconsistent results.

For preserving Pinot semantics we should implement something like https://trino.io/docs/current/connector/postgresql.html#query-varchar-table instead.

in pinot the only way to make distinction between null and non-null values is by using IS NULL and IS NOT NULL other than this pinot will assign the default value if null. Push-down already happens for other types, should this be addressed in a separate ticket? I could take a look at this once we merge this PR. I'll summon @elonazoulay on this one

I tried this for filter pushdown - the behavior is not consistent with trino sql: you can select ... where is null and the result will be non null (i.e. the default value). Will take a look and see what can be done.

yes, the projection in Pinot will always return the default value if column is null

nizarhejazi · 2022-05-20T16:28:46Z

@ddcprg Can you address @hashhar feedback? thanks.

nizarhejazi · 2022-05-24T16:31:39Z

@hashhar @xiangfu0 can you take another look? thanks.

xiangfu0 · 2022-05-24T17:47:41Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/decoders/TimestampDecoder.java

Why write microseconds to this TIMESTAMP_MILLIS?

@nizarhejazi @ddcprg can you check this?
Also make the CI passing?

@xiangfu0 I'm absent at the moment. I'll retake this in 2 weeks time

this is a temporary conversion to micros to be able to invoke TIMESTAMP_MILLIS.writeLong(output, epochMicros); if there is a better way to write milliseconds to the output block please let me know. Please read the javadocs in ShortTimestampType

nizarhejazi · 2022-06-14T14:21:40Z

@ddcprg @xiangfu0 @hashhar @findepi any update on this PR? It has been in review for more than a month and half!

xiangfu0 · 2022-07-06T18:19:31Z

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java

Can you also add a few tests for timestamp predicate and pushdown?
E.g. where timestamp_col > timestamp '2022-01-01 00:00:00.000'

@xiangfu0 thanks for suggesting this test! I found I had to add one more conversion

ddcprg · 2022-07-21T08:28:26Z

@ebyhr I can't reply to your comment about JSON type and using TypeManager. I will have to do a bit of refactoring to use that class, is it fine if we keep it like this for now? We can address in a future PR

ebyhr · 2022-07-21T09:47:45Z

is it fine if we keep it like this for now?

I would defer to @hashhar (I haven't reviewed this PR at all)

hashhar · 2022-07-21T10:35:13Z

Sorry for the delay here. I'll take a look over the weekend.

hashhar · 2022-07-26T06:27:34Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSegmentPageSource.java

only for ShortTimestampType i.e. precision <= 6.

I'll make an additional note

hashhar · 2022-07-26T06:28:28Z

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotSegmentPageSource.java

Does Pinot only support timestamps with precision <= 6? If not we should probably add a verify here to make sure we don't silently do incorrect things with higher precision values.

pinot timestamps are milliseconds, see https://github.com/apache/pinot/blob/master/pinot-spi/src/main/java/org/apache/pinot/spi/data/FieldSpec.java#L468

elonazoulay · 2022-08-02T18:23:20Z

Hi @ddcprg ! I have a pr, we use json support internally here for some time but I just pushed this, didn't realize you are doing something similar: #13428 - would it make sense to just implement the timestamp support in this pr? We have a really good use case for it but did not implement it. lmk if you want to chat offline, I can help test this as well. Also if you could take a look at #13428 whenever you have some time. lmk what works for you.

ddcprg · 2022-08-02T20:10:51Z

@elonazoulay @ebyhr I'm happy to remove the JSON type from this PR, you've done a lot of refactoring in the other PR which what was initially avoiding with this one. This PR has been open for long time though so wonder if we just drop it, unfortunately I don't have plenty of time to dedicate to open source contributions

ddcprg · 2022-08-02T20:11:29Z

@elonazoulay is a bit late for me right now, I'll ping you over the chat tomorrow

xiangfu0 · 2022-08-03T05:03:19Z

+1 on separated the JSON and TIMESTAMP type support

ddcprg · 2022-08-03T07:18:23Z

@xiangfu0 @elonazoulay @ebyhr @hashhar the scope of this PR is now reduced to simple timestamp types, please take a look when you have a min

xiangfu0

lgtm

etadelta222 · 2022-08-29T19:04:55Z

Just wanted to follow-up on this 👀

jatin5251 · 2022-09-05T14:46:53Z

@ddcprg can you please resolve conflicts if possible then we can request for merging thanks

xiangfu0 · 2022-09-07T20:12:21Z

@ddcprg can you please resolve conflicts if possible then we can request for merging thanks

+1

xiangfu0 · 2022-09-16T22:09:45Z

hi @ddcprg, could you please resolve the conflict?

xiangfu0 · 2022-09-21T23:05:03Z

Since I cannot edit @ddcprg 's branch to amend this PR. I took his branch and rebased the code to latest master and create a new PR: #14244

cla-bot bot added the cla-signed label Apr 26, 2022

github-actions bot added the docs label Apr 26, 2022

ddcprg commented Apr 26, 2022

View reviewed changes

ddcprg requested review from ebyhr, elonazoulay, hashhar and xiangfu0 April 26, 2022 22:35

ddcprg commented Apr 26, 2022

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotColumnHandle.java Outdated Show resolved Hide resolved

ebyhr removed their request for review April 26, 2022 23:15

ddcprg force-pushed the issue_10199 branch from 3233354 to 3786149 Compare April 27, 2022 21:06

xiangfu0 reviewed Apr 27, 2022

View reviewed changes

ddcprg force-pushed the issue_10199 branch 3 times, most recently from 5a49dfe to 7a55c31 Compare May 2, 2022 16:28

hashhar reviewed May 15, 2022

View reviewed changes

plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/conversion/PinotTimestamps.java Outdated Show resolved Hide resolved

hashhar reviewed May 15, 2022

View reviewed changes

plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java Outdated Show resolved Hide resolved

hashhar reviewed May 15, 2022

View reviewed changes

ddcprg force-pushed the issue_10199 branch 2 times, most recently from ead0d04 to 50cdb05 Compare May 23, 2022 15:06

xiangfu0 reviewed May 24, 2022

View reviewed changes

ddcprg force-pushed the issue_10199 branch from 50cdb05 to 33f5f78 Compare July 2, 2022 09:55

xiangfu0 reviewed Jul 6, 2022

View reviewed changes

ddcprg force-pushed the issue_10199 branch from 33f5f78 to 4394be3 Compare July 21, 2022 08:25

hashhar self-requested a review July 21, 2022 10:35

ddcprg force-pushed the issue_10199 branch from 4394be3 to d4ac409 Compare July 22, 2022 07:51

hashhar reviewed Jul 26, 2022

View reviewed changes

ddcprg force-pushed the issue_10199 branch from d4ac409 to e3e3834 Compare July 31, 2022 19:19

ddcprg changed the title ~~Add support for timestamp and json types to Pinot connector~~ Add support for timestamp type to Pinot connector Aug 3, 2022

ddcprg force-pushed the issue_10199 branch 2 times, most recently from 97a5116 to 0424a1d Compare August 3, 2022 07:16

xiangfu0 approved these changes Aug 3, 2022

View reviewed changes

Add timestamp support to Pinot connector

d22b30f

ddcprg force-pushed the issue_10199 branch from 0424a1d to d22b30f Compare August 5, 2022 05:50

xiangfu0 requested a review from martint September 7, 2022 20:10

xiangfu0 mentioned this pull request Sep 21, 2022

Add timestamp support to Pinot connector #14244

Merged

ddcprg closed this Sep 26, 2022

Conversation

ddcprg commented Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hashhar commented Apr 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg Apr 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg May 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nizarhejazi commented Apr 29, 2022

Uh oh!

nizarhejazi commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nizarhejazi commented May 10, 2022

Uh oh!

hashhar commented May 11, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddcprg commented Apr 26, 2022 •

edited

Loading

ddcprg Apr 26, 2022 •

edited

Loading

ddcprg Apr 28, 2022 •

edited

Loading

ddcprg May 2, 2022 •

edited

Loading

nizarhejazi commented May 6, 2022 •

edited

Loading

ddcprg May 23, 2022 •

edited

Loading

ddcprg May 23, 2022 •

edited

Loading

ddcprg Jul 31, 2022 •

edited

Loading

xiangfu0 Jul 6, 2022 •

edited

Loading