[CALCITE-5703] Reduce amount of generated runtime code

[zstan]

No description provided.

[DonnyZone]

A nice finding to reduce the generated code. From my perspective, the changes in RexImpTable loss the internal state information during code generation phase, probably leading to some potential risks. Optimizing the code in linq4j (ExpressionWriter & DeclarationStatement & TernaryExpression) may be a better way. How about your opinion?

[DonnyZone]

A more suitable place to conduct such optimizations is Linq4J Optimizer. The similar test cases FYI:(

calcite/linq4j/src/test/java/org/apache/calcite/linq4j/test/OptimizerTest.java

Line 158 in b3e16df

// (v ? (Integer) null : inp0_) == null

)

[zstan]

@DonnyZone @rubenada new approach is force pushed, plz take a look?

[DonnyZone]

The accept function is responsible for generating code string with ExpressionWriter. Instead of rewriting the fields in BinaryExpression and TernaryExpression, we can rewrite in the accept function to narrow the code change.

But the most appropriate place is the OptimizeShuttle, which is specific to optimizes expressions. You can add this case in it.
https://github.com/apache/calcite/blob/main/linq4j/src/main/java/org/apache/calcite/linq4j/tree/OptimizeShuttle.java

[snuyanzin]

As mentioned in jira
removal of such cast could lead to Janino code compiliation problems like janino-compiler/janino#188
So I would suggest to wait for Janino fix for that first

[zstan]

@DonnyZone 3rd reincarnation ) is ready for review,

@snuyanzin if i understand also if PR will be accepted we can`t merge it, do i need to fill the issue for bumping janino version in calcite or you can do it yourself ?

[snuyanzin]

I remember a discussion about that and it was a desicion to not create jira issues just for version updates...
Anyway since I'm involved in the mentioned Janino's issue I could make an update once a release with fix is available (and passes all the tests like Calcite and Flink) and will notify here about that

[zstan]

@DonnyZone thanks for review, but i still not hear any contradictions against this PR can be merged. If it so - is it possible to merge it ?

[DonnyZone]

@DonnyZone thanks for review, but i still not hear any contradictions against this PR can be merged. If it so - is it possible to merge it ?

Sorry for the late reply. I make some tests in my local environment. The optimization for BinaryExpression seems to be incorrect.
The code after optimization throws compilation error.

        Boolean a = true || null;
        Boolean b = null || true;

While the code below works well.

        Boolean c = true || (Boolean) null;
        Boolean d = false || (Boolean) null;
        Boolean e = (Boolean) null && true;

[zstan]

Sorry for the late reply. I make some tests in my local environment. The optimization for BinaryExpression seems to be incorrect. The code after optimization throws compilation error.

        Boolean a = true || null;
        Boolean b = null || true;

@DonnyZone thanks for reply

I can`t obtain such a code after optimization ( can you show me how can i make it ?

final ParameterExpression x_ = Expressions.parameter(Object.class, "x");
BinaryExpression y1 = Expressions.orElse(Expressions.constant(true), Expressions.constant(null));
DeclarationStatement exp0 = Expressions.declare(Modifier.PUBLIC, x_, y1);

assertEquals("{\n public Object x = true;\n}\n",
optimize(exp0))

after optimization shows:
public Object x = true;

[DonnyZone]

Sorry for the late reply. I make some tests in my local environment. The optimization for BinaryExpression seems to be incorrect. The code after optimization throws compilation error.

        Boolean a = true || null;
        Boolean b = null || true;

@DonnyZone thanks for reply

I can`t obtain such a code after optimization ( can you show me how can i make it ?

final ParameterExpression x_ = Expressions.parameter(Object.class, "x"); BinaryExpression y1 = Expressions.orElse(Expressions.constant(true), Expressions.constant(null)); DeclarationStatement exp0 = Expressions.declare(Modifier.PUBLIC, x_, y1);

assertEquals("{\n public Object x = true;\n}\n", optimize(exp0))

after optimization shows: public Object x = true;

I just pick the code from your UT.

  @Test void testOptimizeBinaryNullCasting() {
    // (v ? null : "a") == 1
    assertEquals("{\n  return (v || null) == 1;\n}\n",
        optimize(
            Expressions.equal(
                Expressions.makeBinary(ExpressionType.OrElse,
                    Expressions.parameter(boolean.class, "v"),
                    new ConstantExpression(String.class, null)),
                ONE)));
  }

By the way, is it necessary to apply skipNullCast for the BinaryExpression?

[zstan]

@DonnyZone got it, i refactor a code according to your suggestions and change the tests.

[DonnyZone]

Thanks for the work. Please address the final comment and squash the commits. I will merge it.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

100.0% Coverage
0.0% Duplication

[zstan]

@DonnyZone thanks for suggestions, all done.

[DonnyZone]

LGTM