Skip to content

[CALCITE-3864] Supports CONCAT for variable arguments#1862

Merged
DonnyZone merged 1 commit intoapache:masterfrom
wenhuitang:CALCITE-3864
Mar 27, 2020
Merged

[CALCITE-3864] Supports CONCAT for variable arguments#1862
DonnyZone merged 1 commit intoapache:masterfrom
wenhuitang:CALCITE-3864

Conversation

@wenhuitang
Copy link
Contributor

@wenhuitang wenhuitang commented Mar 19, 2020

@zabetak zabetak added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Mar 23, 2020
@XuQianJin-Stars
Copy link
Contributor

XuQianJin-Stars commented Mar 24, 2020

hi @wenhuitang In addition, you need to add function docs.

@wenhuitang
Copy link
Contributor Author

wenhuitang commented Mar 24, 2020 •
edited
Loading

hi @wenhuitang In addition, you need to add function docs.

The description of CONCAT(string [, string ]*) has already existed in Line 2325 of the document reference.md

@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 25, 2020

Some comments:
(1)In Oracle, concat function accepts only two operands. In SqlLibraryOperators, We'd better seperate the CONCAT_FUNCTION into CONCAT_FUNCTION and ORACLE_CONCAT_FUNCTION. For ORACLE_CONCAT_FUNCTION, we can use SqlOperandCountRanges.of(2) to enable the constraint.
(2)Implement the CONCAT_FUNCTION in SqlFunctions and register the implementation in RexImpTable.
(3)Update the unparse logic accordingly.

@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 25, 2020

There is no confusion. From user's perspective, when the library is specified, the function can be registered correctly. These two functions are placed in two distinct slots.

@wenhuitang wenhuitang force-pushed the CALCITE-3864 branch 2 times, most recently from 703357f to 2cf7349 Compare March 25, 2020 09:21
@wenhuitang
Copy link
Contributor Author

wenhuitang commented Mar 25, 2020

There is no confusion. From user's perspective, when the library is specified, the function can be registered correctly. These two functions are placed in two distinct slots.

According to you comments, this pull request has been updated, thanks a lot.

DonnyZone
DonnyZone reviewed Mar 25, 2020
View reviewed changes
DonnyZone
DonnyZone reviewed Mar 25, 2020
View reviewed changes
@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 25, 2020

Moreover, (1) add several tests in functions.iq; (2) please revise the commit message & JIRA title & PR title as "Implement Concat function".

@wenhuitang wenhuitang changed the title [CALCITE-3864] Add Implementation for SqlLibraryOperators.CONCAT_FUNCTION in SqlFunctions and correct the return type inference of SqlLibraryOperators.CONCAT_FUNCTION [CALCITE-3864] Mar 25, 2020
@wenhuitang wenhuitang changed the title [CALCITE-3864] [CALCITE-3864] Implement Concat function Mar 25, 2020
@wenhuitang
Copy link
Contributor Author

wenhuitang commented Mar 25, 2020

Comments has been addressed, and reference.md has been updated too.

@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 25, 2020

+1, LGTM

@DonnyZone DonnyZone added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR labels Mar 25, 2020
@DonnyZone DonnyZone closed this Mar 26, 2020
@DonnyZone DonnyZone reopened this Mar 26, 2020
@DonnyZone DonnyZone closed this Mar 27, 2020
@DonnyZone DonnyZone reopened this Mar 27, 2020
@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 27, 2020

Close and reopen to trigger the checks.

chunweilei
chunweilei reviewed Mar 27, 2020
View reviewed changes
core/src/test/java/org/apache/calcite/sql/test/SqlOperatorBaseTest.java
tester3.setFor(SqlLibraryOperators.CONCAT2);
tester3.checkString("concat(cast('fe' as char(2)), cast('df' as varchar(65535)))",
"fedf", "VARCHAR NOT NULL");
tester3.checkString("concat(cast('fe' as char(2)), cast('df' as varchar))",
Copy link
Contributor

@chunweilei chunweilei Mar 27, 2020

Choose a reason for hiding this comment

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

Please add a test case whose parameter is empty.

chunweilei
chunweilei reviewed Mar 27, 2020
View reviewed changes
Copy link
Contributor

@chunweilei chunweilei left a comment

Choose a reason for hiding this comment

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

Please also improve the commit message. For example, concat should be upper case. Besides, CONCAT is supported before, but CONCAT with var args is not supported.

@DonnyZone
Copy link
Contributor

DonnyZone commented Mar 27, 2020

In CALCITE-3235, CONCAT function with var args is added but not implemented (i.e., the query can only pass validation).
This PR focuses on:
(1) adding Oracle's CONCAT function, it accepts only two operands
(2) implementing CONCAT function in runtime
(3) fixing the return type inference issue

Personally, I think "Implement CONCAT function" is ok.
@chunweilei @wenhuitang Do you have any suggestions to cover the topics above?

danny0405
danny0405 reviewed Mar 27, 2020
View reviewed changes
core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
/**
* Same as {@link #MULTIVALENT_STRING_SUM_PRECISION} and using
* {@link org.apache.calcite.sql.type.SqlTypeTransforms#TO_NULLABLE}
*/
Copy link
Contributor

@danny0405 danny0405 Mar 26, 2020

Choose a reason for hiding this comment

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

End up the comment with dot.

core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java
*
* concat(cast('a' as varchar(65535)), cast('b' as varchar(2)), cast('c' as varchar(2)))
* returns varchar
*/
Copy link
Contributor

@danny0405 danny0405 Mar 26, 2020

Choose a reason for hiding this comment

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

Start with <p> If there is a blank line ahead. And end with dot.

@danny0405
Copy link
Contributor

danny0405 commented Mar 27, 2020

In CALCITE-3235, CONCAT function with var args is added but not implemented (i.e., the query can only pass validation).
This PR focuses on:
(1) adding Oracle's CONCAT function, it accepts only two operands
(2) implementing CONCAT function in runtime
(3) fixing the return type inference issue

Personally, I think "Implement CONCAT function" is ok.
@chunweilei @wenhuitang Do you have any suggestions to cover the topics above?

"Supports CONCAT for variable arguments" seems fine to me.

@wenhuitang
Copy link
Contributor Author

wenhuitang commented Mar 27, 2020

@DonnyZone @danny0405 @chunweilei Thanks a lot. I have addressed all the comments. And I'm ok with "Supports CONCAT for variable arguments", I will revise the relevant content.

@wenhuitang wenhuitang changed the title [CALCITE-3864] Implement Concat function [CALCITE-3864] Supports CONCAT for variable arguments Mar 27, 2020
@wenhuitang wenhuitang closed this Mar 27, 2020
@wenhuitang wenhuitang reopened this Mar 27, 2020
@DonnyZone DonnyZone merged commit 888dd3a into apache:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danny0405 danny0405 danny0405 left review comments

@XuQianJin-Stars XuQianJin-Stars XuQianJin-Stars left review comments

@DonnyZone DonnyZone DonnyZone left review comments

@chunweilei chunweilei chunweilei left review comments

Assignees

No one assigned

Labels

LGTM-will-merge-soon Overall PR looks OK. Only minor things left.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@wenhuitang @XuQianJin-Stars @DonnyZone @danny0405 @chunweilei @zabetak