[CALCITE-5426] BlockBuilder should not optimize expressions related to mutable objects to variable

• Type: Bug
• Status: Open
• Priority: Major
• Resolution: Unresolved
• Affects Version/s: 1.32.0
• Component/s: linq4j
• Assignee: Unassigned
• Reporter: Dmitry Sysolyatin
• Created: 09/Dec/22 16:02
• Updated: 11/Dec/22 18:50

Description

Inside BlockBuilder there is an optimization that replaces an expression with a variable if the expressions are equal and have the final modifier.

But this optimization can cause problems when used with a mutable objects (One of the problems has been found in CALCITE-5388):

For example [1]:

  @Test void testReuseCollectionExpression() throws NoSuchMethodException {
    Method putMethod = HashMap.class.getMethod("put", Object.class, Object.class);
    Method sizeMethod = HashMap.class.getMethod("size");

    Expression multiMapParent = b.append("multiMap", Expressions.new_(Types.of(HashMap.class)));
    b.add(Expressions.statement(
        Expressions.call(multiMapParent, putMethod, Expressions.box(ONE), Expressions.box(ONE))));

    BlockBuilder nested = new BlockBuilder(true, b);
    Expression multiMapNested = nested.append("multiMap",
        Expressions.new_(Types.of(HashMap.class)));
    nested.add(Expressions.statement(
        Expressions.call(multiMapNested, putMethod, Expressions.box(TWO), Expressions.box(TWO))));
    nested.add(Expressions.call(multiMapNested, sizeMethod));

    b.add(nested.toBlock());
    b.append(Expressions.call(multiMapParent, sizeMethod));

    // It is wrong output. Map should not be reused
    assertEquals(
        "{\n"
            + "  final java.util.HashMap multiMap = new java.util.HashMap();\n"
            + "  multiMap.put(Integer.valueOf(1), Integer.valueOf(1));\n"
            + "  {\n"
            + "    multiMap.put(Integer.valueOf(2), Integer.valueOf(2));\n"
            + "    return multiMap.size();\n"
            + "  }\n"
            + "  return multiMap.size();\n"
            + "}\n",
        b.toBlock().toString());
  }

Are there any performance tests that prove that this optimization significantly improves performance?

[1] https://github.com/dssysolyatin/calcite/commit/626a5f48ef9e69e543aeec277a4f38000a190b10

Issue Links

• is related to: CALCITE-5388 tempList expression inside EnumerableWindow.getPartitionIterator should be unoptimized