[CALCITE-5507] HAVING alias failed when aggregate function in condition#3055
[CALCITE-5507] HAVING alias failed when aggregate function in condition#3055JiajunBernoulli wants to merge 2 commits intoapache:mainfrom
Conversation
JiajunBernoulli
force-pushed
the
CALCITE-5507
branch
2 times, most recently
from
22b5546 to
d1fe017
Compare
NobiGo
left a comment
NobiGo
left a comment
There was a problem hiding this comment.
I think we should validate the SQL result in .iq file.
Make sense, but I don't know how to change SqlConformanceEnum in agg.iq. Would anyone tell me, thanks~ |
|
Hi @JiajunBernoulli |
|
@NobiGo , I added two unit tests in SqlToRelConverter, would you please review it again? |
| * @param parent a SqlNode | ||
| * @param children a SqlIdentifier | ||
| */ | ||
| private boolean containIdentifier(SqlNode parent, SqlIdentifier children) { |
There was a problem hiding this comment.
containIdentifier -> containsIdentifier?
There was a problem hiding this comment.
Good catch, thanks for your review.
|
LGTM |
JiajunBernoulli
force-pushed
the
CALCITE-5507
branch
from
bbaead8 to
01fe96a
Compare
libenchao
left a comment
libenchao
left a comment
There was a problem hiding this comment.
@JiajunBernoulli Thanks for your PR, it looks good generally, I only left some minor comments.
| * @param parent a SqlNode | ||
| * @param children a SqlIdentifier | ||
| */ | ||
| private boolean containsIdentifier(SqlNode parent, SqlIdentifier children) { |
There was a problem hiding this comment.
There is no need to use plural for second parameter. Further more, we even do not need to name them 'parent, child', it's not helpful for readability in my opinion.
How about name it containsIdentifier(SqlNode sqlNode, SqlIdentifier target)?
There was a problem hiding this comment.
Thanks, I renamed them.
| @Test void testAggregateFunAndAliasInHaving2() { | ||
| sql("select count(empno) as e\n" | ||
| + "from emp\n" | ||
| + "having e > 10 or count(empno) < 5") |
There was a problem hiding this comment.
I'm not sure if we need to have a different test which has only a minor difference with testAggregateFunAndAliasInHaving1 (from AND to OR)
There was a problem hiding this comment.
I want to show it because one is reduced and another is not reduced.
testAggregateFunAndAliasInHaving1
LogicalFilter(condition=[>($0, 10)])has noand.testAggregateFunAndAliasInHaving2
LogicalFilter(condition=[SEARCH($0, Sarg[(-∞..5), (10..+∞)])])hasor.
JiajunBernoulli
force-pushed
the
CALCITE-5507
branch
from
dec2f07 to
ddbcf81
Compare
|
Kudos, SonarCloud Quality Gate passed! |
4 participants
No description provided.