Skip to content

Add timestamp support to Pinot connector#14244

Merged
martint merged 1 commit intotrinodb:masterfrom
xiangfu0:timestamp-index
Sep 27, 2022
Merged

Add timestamp support to Pinot connector#14244
martint merged 1 commit intotrinodb:masterfrom
xiangfu0:timestamp-index

Conversation

@xiangfu0
Copy link
Copy Markdown
Contributor

@xiangfu0 xiangfu0 commented Sep 21, 2022 •
edited by martint
Loading

Description

Add support for TIMESTAMP data type in Pinot connector.

Release notes

# Pinot Changes
* Add support for `timestamp` type. (#10199)

@cla-bot cla-bot bot added the cla-signed label Sep 21, 2022
@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Sep 21, 2022

Since #12145 becomes stale, I just took it over and rebased the latest code.
All credit goes to @ddcprg .

@xiangfu0 xiangfu0 added the docs label Sep 21, 2022
@xiangfu0 xiangfu0 requested a review from jatin5251 September 21, 2022 23:04
@xiangfu0 xiangfu0 mentioned this pull request Sep 21, 2022
Closed
@xiangfu0 xiangfu0 force-pushed the timestamp-index branch 2 times, most recently from 15f8080 to 3d46ac7 Compare September 22, 2022 01:30
@ddcprg
Copy link
Copy Markdown
Member

ddcprg commented Sep 22, 2022 •
edited
Loading

@xiangfu0 apologies for the delay and many thanks for rebasing the code. Unfortunately I don't have my personal laptop where I am but I'm happy to either continue the code review on this PR or rebasing your branch into mine next Monday if Trino developers prefer to continue reviewing that instead

Praveen2112
Praveen2112 reviewed Sep 22, 2022
View reviewed changes
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java Outdated
Comment on lines 1556 to 1562
Copy link
Copy Markdown
Member

@Praveen2112 Praveen2112 Sep 22, 2022

Choose a reason for hiding this comment

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

I think we don't need this test doesn't support SUPPORTS_CREATE_TABLE behavior.

Copy link
Copy Markdown
Contributor Author

@xiangfu0 xiangfu0 Sep 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The base class don't have the check for SUPPORTS_CREATE_TABLE, so need o override here.

Copy link
Copy Markdown
Member

@ebyhr ebyhr Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No need to override. The base class has the behavior check.

trino/testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java

Lines 128 to 134 in 02fabc7

@Test
public void testCreateTable()
{
if (!hasBehavior(SUPPORTS_CREATE_TABLE)) {
assertQueryFails("CREATE TABLE xxxx (a bigint, b double)", "This connector does not support creating tables");
return;
}

@xiangfu0 xiangfu0 force-pushed the timestamp-index branch 2 times, most recently from 88d9b2b to fee1273 Compare September 22, 2022 08:06
@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Sep 22, 2022 •
edited
Loading

@xiangfu0 apologies for the delay and many thanks for rebasing the code. Unfortunately I don't have my personal laptop where I am but I'm happy to either continue the code review on this PR or rebasing your branch into mine next Monday if Trino developers prefer to continue reviewing that instead

Since there are a bunch of refactors happened, I would just go with this PR.

@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Sep 22, 2022

Hi @Praveen2112 , thanks for your review. Can u do another pass?

@xiangfu0 xiangfu0 force-pushed the timestamp-index branch 3 times, most recently from 3762bd3 to a83f08b Compare September 23, 2022 23:35
@xiangfu0 xiangfu0 requested a review from martint September 24, 2022 07:04
ddcprg
ddcprg approved these changes Sep 26, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@ddcprg ddcprg left a comment

Choose a reason for hiding this comment

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

thanks for rebasing and reviving the PR

@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Sep 26, 2022

cc: @ebyhr as well

@martint martint merged commit 5ff14f1 into trinodb:master Sep 27, 2022
@xiangfu0 xiangfu0 deleted the timestamp-index branch September 27, 2022 00:30
@xiangfu0
Copy link
Copy Markdown
Contributor Author

xiangfu0 commented Sep 27, 2022

Many thanks @martint !

@github-actions github-actions bot added this to the 398 milestone Sep 27, 2022
ebyhr
ebyhr reviewed Sep 27, 2022
View reviewed changes
.../trino-pinot/src/test/java/io/trino/plugin/pinot/BasePinotIntegrationConnectorSmokeTest.java
Comment on lines +1552 to +1555
public void testPredicatePushdown()
{
assertThat(query("SELECT timestamp_col FROM " + ALL_TYPES_TABLE + " WHERE timestamp_col < TIMESTAMP '1971-01-01 00:00:00.000'"))
.isFullyPushedDown();
Copy link
Copy Markdown
Member

@ebyhr ebyhr Sep 27, 2022

Choose a reason for hiding this comment

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

The method name is testPredicatePushdown but the test is specific to timestamp type. Moving to testTimestamp seems better.

@xiangfu0 xiangfu0 mentioned this pull request Sep 27, 2022
Merged
@colebow colebow mentioned this pull request Sep 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ebyhr ebyhr ebyhr left review comments

@findepi findepi Awaiting requested review from findepi

@elonazoulay elonazoulay Awaiting requested review from elonazoulay

@hashhar hashhar Awaiting requested review from hashhar

@jatin5251 jatin5251 Awaiting requested review from jatin5251

@Praveen2112 Praveen2112 Awaiting requested review from Praveen2112

@martint martint Awaiting requested review from martint

+1 more reviewer

@ddcprg ddcprg ddcprg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed docs

Milestone

398

Development

Successfully merging this pull request may close these issues.

5 participants

@xiangfu0 @ddcprg @martint @ebyhr @Praveen2112