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-3319] AssertionError when reducing decimals #1442

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

yanlin-Lynn
Copy link
Contributor

There exist code that throws AssertionError in ReduceDecimalsRule every time it was ran to.

List newOperands = apply(call.getOperands());
if (true) {
// FIXME: Operands are now immutable. Create a new call with
// new operands?
throw new AssertionError();
}

This PR tries to remove this part of code, and add unit test case for the new code path.
Please refer to CALCITE-3319 for full stack trace.

@yanlin-Lynn yanlin-Lynn force-pushed the reduce-decimal branch 2 times, most recently from cfbdcbc to acb50a0 Compare September 6, 2019 23:04
Copy link
Member

@hsyuan hsyuan left a comment

Choose a reason for hiding this comment

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

+1, but recommend changing the title to AssertionError when reducing decimals so that the title can describe what is the problem this PR is trying to fix.

@hsyuan hsyuan added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Sep 7, 2019
There exist code that throws AssertionError every time it was ran to,
remove this part of code and add test for new code path
@yanlin-Lynn yanlin-Lynn changed the title [CALCITE-3319] Remove throwing AssertionError code in ReduceDecimalsRule [CALCITE-3319] AssertionError when reducing decimals Sep 7, 2019
@yanlin-Lynn
Copy link
Contributor Author

+1, but recommend changing the title to AssertionError when reducing decimals so that the title can describe what is the problem this PR is trying to fix.

OK. I update the title of this PR and the commit log.

@hsyuan hsyuan merged commit ce8ca67 into apache:master Sep 12, 2019
@yanlin-Lynn yanlin-Lynn deleted the reduce-decimal branch September 13, 2019 01:25
@yanlin-Lynn
Copy link
Contributor Author

Thanks @hsyuan and @danny0405 !

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants