Skip to content

[CALCITE-5486] SubQuery not support HAVING alias in where condition#3053

Open
JiajunBernoulli wants to merge 3 commits intoapache:mainfrom
JiajunBernoulli:CALCITE-5486
Open

[CALCITE-5486] SubQuery not support HAVING alias in where condition#3053
JiajunBernoulli wants to merge 3 commits intoapache:mainfrom
JiajunBernoulli:CALCITE-5486

Conversation

@JiajunBernoulli
Copy link
Contributor

No description provided.

@Override public @Nullable SqlNode visit(SqlCall call) {
call.getOperandList()
.stream()
.filter(node -> node != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

node -> node != null -> Objects::nonNull seems better, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good advice.

/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-5486">[CALCITE-5486]
* SubQuery not support HAVING alias in where condition</a>. */
@Test void testHavingAliasInCondition() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also add a test in SqlToRelConverterTest to ensure we get the right plan?

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, done.

@zoudan
Copy link
Contributor

zoudan commented Feb 6, 2023

LGTM

.forEach(node -> node.accept(this));
if (call.getKind() == SqlKind.SELECT) {
SqlSelect select = (SqlSelect) call;
validateHavingClause(select);
Copy link
Member

Choose a reason for hiding this comment

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

validateWhereOrOn is used not only to validate where clause, is your change also applies to on clause, is there any test covers this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, I added one test.

@sonarqubecloud
Copy link

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

@libenchao libenchao added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Feb 21, 2023
@libenchao
Copy link
Member

@JiajunBernoulli I've left some comments in the jira, and marked this PR "discussion in jira" label.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

@github-actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.

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.

3 participants