Skip to content

[CALCITE-3724] Presto dialect implementation#1776

Closed
XuQianJin-Stars wants to merge 2 commits intoapache:masterfrom
XuQianJin-Stars:CALCITE-3724
Closed

[CALCITE-3724] Presto dialect implementation#1776
XuQianJin-Stars wants to merge 2 commits intoapache:masterfrom
XuQianJin-Stars:CALCITE-3724

Conversation

@XuQianJin-Stars
Copy link
Contributor

@XuQianJin-Stars XuQianJin-Stars commented Jan 30, 2020

As illustrated in CALCITE-3724

@XuQianJin-Stars XuQianJin-Stars requested review from danny0405 and hsyuan and removed request for hsyuan January 31, 2020 07:43
@XuQianJin-Stars XuQianJin-Stars changed the title [CALCITE-3724] Implement PrestoSqlDialect [CALCITE-3724] Implement Presto Dialect Feb 4, 2020
@XuQianJin-Stars XuQianJin-Stars changed the title [CALCITE-3724] Implement Presto Dialect [CALCITE-3724] Presto dialect implementation Feb 4, 2020
danny0405
danny0405 reviewed Feb 7, 2020
View reviewed changes
Copy link
Contributor

@danny0405 danny0405 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 the PR, can you explain why we need a Presto dialect ? Which unparse logic do you want to customize ?

@XuQianJin-Stars
Copy link
Contributor Author

XuQianJin-Stars commented Feb 7, 2020 •
edited
Loading

Thanks for the PR, can you explain why we need a Presto dialect ? Which unparse logic do you want to customize ?

hi @danny0405 The previous discussion is here [1]
[1] https://mail-archives.apache.org/mod_mbox/calcite-dev/202001.mbox/%3cCAPSgeETHa88DN7Bwqd+qez-i1=3XDa2LN3+wQdk7x_QfEXm8Ew@mail.gmail.com%3e

@danny0405
Copy link
Contributor

danny0405 commented Feb 12, 2020 •
edited
Loading

Thanks for the PR, can you explain why we need a Presto dialect ? Which unparse logic do you want to customize ?

hi @danny0405 The previous discussion is here [1]
[1] https://mail-archives.apache.org/mod_mbox/calcite-dev/202001.mbox/%3cCAPSgeETHa88DN7Bwqd+qez-i1=3XDa2LN3+wQdk7x_QfEXm8Ew@mail.gmail.com%3e

Thanks, you can paste the conclusion/the background to the JIRA issue or the link is okey.

@DonnyZone
Copy link
Contributor

DonnyZone commented Apr 7, 2020

Hi, @XuQianJin-Stars, could you please rebase the PR? It will be nice if you can provide presto's doc and highlight the unparse logic in Jira/PR's description.

@XuQianJin-Stars
Copy link
Contributor Author

XuQianJin-Stars commented Apr 8, 2020

Hi, @XuQianJin-Stars, could you please rebase the PR? It will be nice if you can provide presto's doc and highlight the unparse logic in Jira/PR's description.

well, but I am a bit busy lately, I will take time to deal with this later.

DonnyZone
DonnyZone reviewed Jun 2, 2020
View reviewed changes
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java Outdated
}

@Test void testCubeInSpark() {
@Test public void testCubeWithGroupBy() {
Copy link
Contributor

@DonnyZone DonnyZone Jun 2, 2020

Choose a reason for hiding this comment

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

I remember that there was a PR that removed public. We'd better follow this style.

Copy link
Contributor Author

@XuQianJin-Stars XuQianJin-Stars Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I remember that there was a PR that removed public. We'd better follow this style.

well, I have changed it.

@DonnyZone
Copy link
Contributor

DonnyZone commented Jun 4, 2020

LGTM

danny0405
danny0405 approved these changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

+1, but needs some refactoring.

@danny0405 danny0405 added the accepted-and-would-rework-soon This patch is overall ok, but need some refactoring by the committers. label Jun 8, 2020
@XuQianJin-Stars
Copy link
Contributor Author

XuQianJin-Stars commented Jun 10, 2020

+1, but needs some refactoring.

hi @danny0405 What are the main contents of rework?

@danny0405 danny0405 closed this Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DonnyZone DonnyZone DonnyZone left review comments

@danny0405 danny0405 danny0405 approved these changes

@hsyuan hsyuan Awaiting requested review from hsyuan

Assignees

No one assigned

Labels

accepted-and-would-rework-soon This patch is overall ok, but need some refactoring by the committers.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@XuQianJin-Stars @danny0405 @DonnyZone