Skip to content

[CALCITE-5265] Select operator' parentheses should be same with Union operator#2910

Closed
l4wei wants to merge 3 commits into
apache:mainfrom
l4wei:CALCITE-5265
Closed

[CALCITE-5265] Select operator' parentheses should be same with Union operator#2910
l4wei wants to merge 3 commits into
apache:mainfrom
l4wei:CALCITE-5265

Conversation

@l4wei
Copy link
Copy Markdown
Contributor

@l4wei l4wei commented Sep 17, 2022

remove necessary parentheses on select clause.

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5265">[CALCITE-5265]
* Select operator' parentheses should be same with Union operator</a>. */
@Test void testInsertValueWithDynamicParams() {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there any tests which cover what Julian has mentioned in the JIRA comment?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There are many unit tests cover "insert into aTable select ? from bTable" in SqlParserTest.java. And RelToSqlConverterTest::testInsertValuesWithDynamicParams will cover "insert into aTable select ? from bTable union all select ? from cTable" mentioned by Julian in JIRA comment.


/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5265">[CALCITE-5265]
* Select operator' parentheses should be same with Union operator</a>. */
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The comment should also be adapted according to the changes in JIRA title.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

Copy link
Copy Markdown
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

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

@l4wei Thanks the updating, the PR LGTM now, merging.

@libenchao
Copy link
Copy Markdown
Member

@l4wei I didn't notice Julian has another comment when I commented with "LGTM", could you add another test which addresses Julian's comment?

@l4wei
Copy link
Copy Markdown
Contributor Author

l4wei commented Sep 28, 2022

OK

@l4wei
Copy link
Copy Markdown
Contributor Author

l4wei commented Sep 28, 2022

I have added test that mentioned by Julian' last comment in third commit in this PR. FYI

Copy link
Copy Markdown
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

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

LGTM, will merge in 24 hours if there is no more objections appear.

@libenchao libenchao closed this in b16df01 Sep 29, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 11, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants