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.

Sign up for GitHub

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

Jump to bottom

[CALCITE-4420] Some simple arithmetic operations can be simplified #2282

Merged

liyafan82 merged 1 commit into apache:master from liyafan82:fly_1126_art

Jul 12, 2021

Contributor

liyafan82 commented Nov 27, 2020

Some simple arithmetic operations can be simplified, for example:

a + 0 = a
a - 0 = a
a * 1 = a
a / 1 = a

In our product, we found such scenarios are fairly frequently encountered. For example, some SQL programmers use a * 1.0 as a convenient way to cast a to floating point type.

liyafan82 force-pushed the fly_1126_art branch 3 times, most recently from 1f832ee to 1633330 Compare

November 27, 2020 06:23

chunweilei reviewed

View reviewed changes

core/src/main/java/org/apache/calcite/rex/RexSimplify.java Outdated

+                  if (RexUtil.isNullLiteral(e.operands.get(0), true)
+                      || RexUtil.isNullLiteral(e.operands.get(1), true)) {
+                    return rexBuilder.makeNullLiteral(e.type);
+                  }

Contributor

chunweilei Nov 28, 2020 •

edited

It might not be true. Sometimes users may want it to throw an exception.

Contributor Author

liyafan82 Nov 30, 2020

Thanks for your feedback. It makes sense. This logic is removed.

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

+                  if (zeroIndex == 1) {
+                    RexNode leftOperand = e.getOperands().get(0);
+                    return leftOperand.getType().equals(e.getType())
+                        ? leftOperand : rexBuilder.makeCast(e.getType(), leftOperand);

Contributor

chunweilei Nov 28, 2020

Can a - a be simplified to 0 if a is not nullable?

Contributor Author

liyafan82 Nov 30, 2020

Thanks for the suggestion.
There are some special cases for which this is not true. For example,

NaN - NaN = NaN
Inf - Inf = NaN

liyafan82 force-pushed the fly_1126_art branch from 1633330 to 68f497f Compare

November 30, 2020 04:48

chunweilei reviewed

View reviewed changes

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

+                private RexNode simplifyMultiply(RexCall e) {
+                  int oneIndex = findLiteralIndex(e.operands, 1L);
+                  if (oneIndex >= 0) {

Contributor

chunweilei Dec 1, 2020

Make it final since it's our convention.

Contributor Author

liyafan82 Dec 1, 2020

Revised. Thanks for the good suggestion.

chunweilei reviewed

View reviewed changes

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

+                      || e.getOperands().stream()
+                      .anyMatch(o -> e.getType().getSqlTypeName().getFamily() != SqlTypeFamily.NUMERIC)) {
+                    // we only support simplifying numeric types
+                    return simplifyGenericNode(e);

Contributor

chunweilei Dec 1, 2020

Add . to the end of the sentence.

Contributor Author

liyafan82 Dec 1, 2020

Done. Thanks.

chunweilei reviewed

View reviewed changes

core/src/main/java/org/apache/calcite/rex/RexSimplify.java

+                      Comparable comparable = ((RexLiteral) operands.get(i)).getValue();
+                      if (comparable instanceof BigDecimal && ((BigDecimal) comparable).longValue() == value) {
+                        return i;
+                      }

Contributor

chunweilei Dec 1, 2020

Could you mark it that it only supports numeric type?

Contributor Author

liyafan82 Dec 1, 2020

Sure. I've made it explicit in the JavaDoc.

liyafan82 force-pushed the fly_1126_art branch 2 times, most recently from 00b866e to 1edcd5a Compare

December 1, 2020 06:25

Member

Aaaaaaron commented Dec 5, 2020

LGTM

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare

December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare

March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare

March 24, 2021 09:43

liyafan82 force-pushed the fly_1126_art branch from 1edcd5a to 11add5b Compare

June 25, 2021 10:22

Contributor Author

liyafan82 commented Jun 25, 2021

Dear reviewers, thanks a lot for your good comments.
If there are no other comments, I am going to merge this in a few days.

liyafan82 added the LGTM-will-merge-soon label

vlsi reviewed

View reviewed changes

core/src/test/java/org/apache/calcite/rex/RexProgramTest.java Show resolved Hide resolved


          [CALCITE-4420] Some simple arithmetic operations can be simplified

2e2f331

liyafan82 force-pushed the fly_1126_art branch from 11add5b to 2e2f331 Compare

June 28, 2021 04:42

liyafan82 removed the LGTM-will-merge-soon label

liyafan82 added the LGTM-will-merge-soon label

Contributor Author

liyafan82 commented Jul 6, 2021

Dear reviewers, thanks a lot for your comments.
If there are no more comments, I want to merge it in a few days.

Contributor

NobiGo commented Jul 6, 2021

LGTM

liyafan82 merged commit c700e37 into apache:master

liyafan82 deleted the fly_1126_art branch

July 12, 2021 10:47

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment