Skip to content

[CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric#3038

Closed
libenchao wants to merge 3 commits intoapache:mainfrom
libenchao:5483-ProjectAggregateMergeRule-throws-casting-exception
Closed

[CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric#3038
libenchao wants to merge 3 commits intoapache:mainfrom
libenchao:5483-ProjectAggregateMergeRule-throws-casting-exception

Conversation

@libenchao
Copy link
Copy Markdown
Member

This PR is based on #3031 from @birschick-bq

== SqlKind.INPUT_REF
&& operands.get(2).getKind() == SqlKind.LITERAL) {
&& operands.get(2).getKind() == SqlKind.LITERAL
&& operands.get(2).getType().getFamily() == SqlTypeFamily.NUMERIC) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optionally, you could move this test (operands.get(2).getType().getFamily() == SqlTypeFamily.NUMERIC) to just before the call to literal.getValueAs(BigDecimal.class) - it might have better context.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was meant to remove this check originally since we can assume the operand is numeric when the aggregator is SUM. Sorry that this was still in my git staging.

@sonarqubecloud
Copy link
Copy Markdown

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

83.3% 83.3% Coverage
0.0% 0.0% Duplication

Copy link
Copy Markdown
Contributor

@birschick-bq birschick-bq left a comment

Choose a reason for hiding this comment

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

LGTM

@libenchao
Copy link
Copy Markdown
Member Author

@birschick-bq Thanks for your review.

I'll merge this after '1.33.0' is out if there is no more objections anymore. And I'll add @birschick-bq as the co-author since this PR is mostly based on his #3031.

@libenchao libenchao closed this in 1a54261 Feb 9, 2023
gleonSun pushed a commit to gleonSun/calcite that referenced this pull request Dec 20, 2023
…is non-numeric

Close apache#3038

Co-authored-by: Bruce Irschick <brucei@bitquilltech.com>
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