Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add date support to kudu #20086

Closed
wants to merge 3 commits into from
Closed

Add date support to kudu #20086

wants to merge 3 commits into from
+45 −8

Conversation

pgasp
Copy link
Contributor

@pgasp pgasp commented Dec 12, 2023 •
edited
Loading

Add support to Kudu
update Test to support Date type

Description

Adding Date Support to Kudu Connector
Test updated accordingly

Additional context and related issues

related to PR : #11009

Release notes

( ) 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.
( x) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@pgasp
fix date kudu
cd2b59b 
Add support to Kudu
update Test to support Date type
@cla-bot cla-bot bot added the cla-signed label Dec 12, 2023
@ebyhr
Copy link
Member

ebyhr commented Dec 13, 2023

Could you confirm CI failure?

@pgasp
Copy link
Contributor Author

pgasp commented Dec 13, 2023

Could you confirm CI failure?

Yes, I do. I'm working on it.

ebyhr
ebyhr reviewed Dec 14, 2023
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Please capitalize the commit message and squash commits into one. https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#format-git-commit-messages

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java Show resolved Hide resolved
plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java Show resolved Hide resolved
plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java
@@ -84,7 +84,7 @@ public static org.apache.kudu.Type toKuduClientType(Type type)
return org.apache.kudu.Type.DECIMAL;
}
if (type == DateType.DATE) {
return org.apache.kudu.Type.STRING;
return org.apache.kudu.Type.DATE;
Copy link
Member

Choose a reason for hiding this comment

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

Older Kudu versions don't support DATE type, right? What's the EOL of those versions?
We should use the different type mapping based on the server version if this change breaks existing environments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The trino kudu connector has dependency of kudu version 1.15.0 whitch supporting date. End users do not understand why trino kudu connector does not support what their server support with kudu client 1.15.0.
I don't have EOL for kudu

Copy link
Member

@ebyhr ebyhr Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The internal dependency is unrelated to users. Our official documentation mentions 1.13.0 is the minimum supported version. I would recommend making sure the EOL.

Copy link
Member

Choose a reason for hiding this comment

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

I think Kudu supports date from 1.12.0 https://kudu.apache.org/docs/prior_release_notes.html#rn_1.12.0 so I guess we are good here. WDYT @ebyhr ?

Copy link
Member

Choose a reason for hiding this comment

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

👍

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java Show resolved Hide resolved
plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java Show resolved Hide resolved
plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java
Comment on lines +612 to +615
assertUpdate(format("INSERT INTO %s VALUES (DATE '-0001-01-01')", table.getName()), 1);
}
catch (Exception error) {
assertThat(error).hasMessageContaining("is out of range");
Copy link
Member

Choose a reason for hiding this comment

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

Please revert and use assertQueryFails.

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java Show resolved Hide resolved
plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/TypeHelper.java
Comment on lines +215 to +217
if (type.equals(DateType.DATE)) {
return row.getInt(field);
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this condition really needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's. Otherwise, anyother type will be picked and that throw and exception. That my guess. isn't it ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think the method takes DATE type.

plugin/trino-kudu/src/main/java/io/trino/plugin/kudu/KuduPageSink.java
@@ -16,6 +16,7 @@
import com.google.common.collect.ImmutableList;
import io.airlift.slice.Slice;
import io.trino.spi.Page;
import io.trino.spi.TrinoException;
Copy link
Member

Choose a reason for hiding this comment

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

Please update type mapping section in kudu.md.

plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduConnectorTest.java
@@ -182,7 +182,7 @@ protected MaterializedResult getDescribeOrdersResult()
.row("custkey", "bigint", extra, "")
.row("orderstatus", "varchar", extra, "")
.row("totalprice", "double", extra, "")
.row("orderdate", "varchar", extra, "")
.row("orderdate", "date", extra, "")
Copy link
Member

Choose a reason for hiding this comment

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

Please update filterDataMappingSmokeTestData.

@ebyhr ebyhr mentioned this pull request Dec 20, 2023
Closed
@ebyhr
Copy link
Member

ebyhr commented Dec 20, 2023

Please rebase on master to resolve conflicts.

@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jan 10, 2024
@mosabua
Copy link
Member

mosabua commented Jan 11, 2024

👋 @pgasp could you please rebase the PR and address any comments so we can proceed with review towards merge.

@github-actions github-actions bot removed the stale label Jan 11, 2024
@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Feb 20, 2024
@github-actions github-actions bot removed the stale label Mar 5, 2024
@github-actions GitHub Actions
Copy link

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Mar 27, 2024
@github-actions GitHub Actions
Copy link

Closing this pull request, as it has been stale for six weeks. Feel free to re-open at any time.

@github-actions github-actions bot closed this Apr 17, 2024
@martint martint reopened this Apr 26, 2024
@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Apr 26, 2024
@ebyhr
Copy link
Member

ebyhr commented Oct 17, 2024

Superseded by #22497

@ebyhr ebyhr closed this Oct 17, 2024
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

@Praveen2112 Praveen2112 Praveen2112 left review comments

Assignees

@pgasp pgasp

Labels
cla-signed stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pgasp @ebyhr @mosabua @Praveen2112 @martint