Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CALCITE-3979] Simplification might have removed CAST expressi… #1969

Merged
merged 1 commit into from May 14, 2020

Conversation

kgyrtkirk
Copy link
Member

…ncorrectly

ReduceExpressionsRule might have removed cast expressions and inadvertly changed the type of some subexpressions.
In case the rewriting have at some point arrived at: CAST(CAST(X,T),T) AND <(CAST(X,T),N)

…ncorrectly

ReduceExpressionsRule might have removed cast expressions and inadvertly changed the type of some subexpressions.
In case the rewriting have at some point arrived at: CAST(CAST(X,T),T) AND <(CAST(X,T),N)
@kgyrtkirk
Copy link
Member Author

@chunweilei , @julianhyde : any objections against removing this logic from ReduceExpressionsRule?

@kgyrtkirk kgyrtkirk added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label May 10, 2020
@kgyrtkirk kgyrtkirk changed the title [CALCITE-3979] Simplifica might have removed CAST expressi… [CALCITE-3979] Simplification might have removed CAST expressi… May 10, 2020
@julianhyde
Copy link
Contributor

@kgyrtkirk As I said in the JIRA case, I really don't know what's safe to remove. It seems that there is a bunch of code to remove casts and there may not be a sound basis for that code.

@kgyrtkirk
Copy link
Member Author

I don't see any reason to keep this logic any longer.
@julianhyde Could you please share your concerns regarding this change?

Copy link
Contributor

@danny0405 danny0405 left a comment

Choose a reason for hiding this comment

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

+1 for this change.

@kgyrtkirk kgyrtkirk merged commit ffa22f7 into apache:master May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LGTM-will-merge-soon Overall PR looks OK. Only minor things left. slow-tests-needed
Projects
None yet
4 participants