Skip to content

[CALCITE-5884] ARRAY_TO_STRING function should return NULL if its 'nullValue' argument is NULL#3346

Merged
libenchao merged 1 commit into
apache:mainfrom
mihaibudiu:issue5884
Nov 6, 2023
Merged

[CALCITE-5884] ARRAY_TO_STRING function should return NULL if its 'nullValue' argument is NULL#3346
libenchao merged 1 commit into
apache:mainfrom
mihaibudiu:issue5884

Conversation

@mihaibudiu
Copy link
Copy Markdown
Contributor

No description provided.

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

@zoudan I think you have implemented this function.

@mihaibudiu mihaibudiu force-pushed the issue5884 branch 3 times, most recently from a23b2fe to b320d7d Compare August 2, 2023 23:24
@rubenada rubenada added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Aug 3, 2023
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Aug 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

I would appreciate if someone could review this PR.

@LakeShen
Copy link
Copy Markdown
Contributor

@mihaibudiu ,please rebase the latest main branch's code.

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

@mihaibudiu ,please rebase the latest main branch's code.

Done

Comment thread core/src/main/java/org/apache/calcite/sql/SqlNumericLiteral.java Outdated
Comment thread core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java Outdated
Comment thread core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java
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.

Thanks @mihaibudiu, it looks good to me with only nit comments. Please note that Julian has proposed a better summary in Jira.

public static String arrayToString(List list, String delimiter, @Nullable String nullText) {
// Note that the SQL function ARRAY_TO_STRING that we implement will return
// 'NULL' when the nullText argument is NULL. However, that is handled by
// the nullPolicy of the RexToLixTranslator. So here a NULL value
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.

NIT: redundant spaces (and in above line too)

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.

I have changed the summary, removed the spaces, and squashed the commits.

…llValue' argument is NULL

Signed-off-by: Mihai Budiu <mbudiu@feldera.com>
@mihaibudiu mihaibudiu changed the title [CALCITE-5884] Type Inference rule for ARRAY_TO_STRING is incorrect [CALCITE-5884] ARRAY_TO_STRING function should return NULL if its 'nullValue' argument is NULL Nov 2, 2023
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.

+1

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

@julianhyde I hope I have made the changes you requested

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud Bot commented Nov 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell D 2 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@libenchao
Copy link
Copy Markdown
Member

Can we move on with merging, or we can leave it out of 1.36.0? I plan to start the release process if there is still no progress on this PR in a day :)

@mihaibudiu
Copy link
Copy Markdown
Contributor Author

I think we should just merge it. I think it's a positive improvement in documentation.

@libenchao libenchao merged commit 4c9011c into apache:main Nov 6, 2023
@mihaibudiu mihaibudiu deleted the issue5884 branch November 6, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants