Skip to content

[CALCITE-5267] Remove unused variable 'newCasts' in AggregateCaseToF… - #2895

Closed
zhuwenzhuang wants to merge 2 commits into
apache:mainfrom
zhuwenzhuang:CALCITE-5267
Closed

[CALCITE-5267] Remove unused variable 'newCasts' in AggregateCaseToF…#2895
zhuwenzhuang wants to merge 2 commits into
apache:mainfrom
zhuwenzhuang:CALCITE-5267

Conversation

@zhuwenzhuang

@zhuwenzhuang zhuwenzhuang commented Sep 6, 2022

Copy link
Copy Markdown
Contributor

Remove unused variable 'newCasts' in AggregateCaseToFilter.

@chunweilei chunweilei left a comment

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.

I checked the history. The rule is introduced from Druid[1], in which newCasts is used. It may be better to correct it instead of removing it.

[1] https://github.com/apache/druid/pull/4360/files#diff-809e3a73d1485dad83629598ea5bcbab8f5ce7724e9572975b052176c44fbdf7

@zhuwenzhuang

Copy link
Copy Markdown
Contributor Author

I checked the history. The rule is introduced from Druid[1], in which newCasts is used. It may be better to correct it instead of removing it.

[1] https://github.com/apache/druid/pull/4360/files#diff-809e3a73d1485dad83629598ea5bcbab8f5ce7724e9572975b052176c44fbdf7

I guess the code "relBuilder.aggregate(groupKey, newCalls).convert(aggregate.getRowType(), false)" is a better way to do CAST. So we don't need to save every project to keep types stable.

@libenchao libenchao left a comment

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.

LGTM, Thanks for your contribution @zhuwenzhuang, I'll merge this if there is no objections from @chunweilei

@libenchao libenchao closed this in 4805478 Sep 20, 2022
@libenchao libenchao changed the title [CALCITE-5267] Remove useless variable 'newCasts' in AggregateCaseToF… [CALCITE-5267] Remove unused variable 'newCasts' in AggregateCaseToF… Sep 20, 2022
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 11, 2022
…lterRule

We now use RelBuilder#convert to make casts.

This closes apache#2895
tanclary pushed a commit to tanclary/calcite that referenced this pull request Nov 28, 2022
…lterRule

We now use RelBuilder#convert to make casts.

This closes apache#2895
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.

3 participants