fix: calcite optimization adds LITERAL_AGG(true)#963
Conversation
Signed-off-by: matthew brian white <whitemat@uk.ibm.com>
Signed-off-by: matthew brian white <whitemat@uk.ibm.com>
|
for some reason your PR header had a trailing whitespace in it |
|
thanks @nielspardon |
|
Nice fix — the approach is correct: within a grouped aggregate every group is non-empty, so One bug, which I reproduced locally (PR branch, JDK 17): The wrapper's passthrough references are typed with the whole aggregate struct. i.e. all five passthroughs are the whole struct; only the re-inserted - projectExprs.add(FieldReference.newRootStructReference(i, aggRel.getRecordType()));
+ projectExprs.add(FieldReference.newInputRelReference(i, aggRel));
@@
- projectExprs.add(
- FieldReference.newRootStructReference(realAggIndex, aggRel.getRecordType()));
+ projectExprs.add(FieldReference.newInputRelReference(realAggIndex, aggRel));With that change the columns become The current test's structural assertions check the outer SELECT projection, not the wrapper, so they pass even with the bug. Suggested replacement that actually inspects the wrapper, plus a guard test for the @Test
void conversionHandlesLiteralAggInsertedBySubQueryRemoveRule()
throws SqlParseException, IOException {
String query =
"select e1.l_orderkey from lineitem e1 "
+ "where e1.l_quantity <> some ("
+ " select l_quantity from lineitem e2 where e2.l_partkey = e1.l_partkey)";
RelRoot relRoot = SubstraitSqlToCalcite.convertQuery(query, TPCH_CATALOG);
HepPlanner hepPlanner =
new HepPlanner(
new HepProgramBuilder()
.addRuleInstance(CoreRules.FILTER_SUB_QUERY_TO_CORRELATE)
.build());
hepPlanner.setRoot(relRoot.rel);
RelNode decorrelated =
RelDecorrelator.decorrelateQuery(
hepPlanner.findBestExp(),
RelFactories.LOGICAL_BUILDER.create(relRoot.rel.getCluster(), null));
// Pin the trigger so a future Calcite bump can't silently stop exercising this path.
assertTrue(containsLiteralAgg(decorrelated), "test setup no longer produces LITERAL_AGG");
Plan.Root planRoot =
assertDoesNotThrow(
() ->
SubstraitRelVisitor.convert(
RelRoot.of(decorrelated, relRoot.kind), EXTENSION_COLLECTION));
// The fix inserts a Project directly over the Aggregate; inspect THAT, not the outer SELECT.
Project wrapper =
findProjectOverAggregate(planRoot.getInput())
.orElseThrow(() -> new AssertionError("expected a Project wrapping the Aggregate"));
assertTrue(
wrapper.getExpressions().stream()
.anyMatch(e -> e instanceof Expression.BoolLiteral && ((Expression.BoolLiteral) e).value()),
"LITERAL_AGG(true) should be re-inserted as a boolean true literal");
// Passthroughs must carry the scalar field type, not the whole aggregate struct.
assertTrue(
wrapper.getRecordType().fields().stream().noneMatch(f -> f instanceof Type.Struct),
"wrapper columns must be scalar; fields=" + wrapper.getRecordType().fields());
// The wrapper subtree must survive a proto round-trip with its schema intact.
ExtensionCollector ec = new ExtensionCollector();
io.substrait.proto.Rel proto = new RelProtoConverter(ec).toProto(wrapper);
Rel rt = new ProtoRelConverter(ec, extensions).from(proto);
assertEquals(wrapper.getRecordType(), rt.getRecordType(), "wrapper schema must round-trip");
}
@Test
void literalAggCombinedWithGroupingSetsIsRejected() {
RelNode input = builder.values(new String[] {"a", "b"}, 1, 2, 3, 4).build();
RexBuilder rexBuilder = creator.rex();
AggregateCall literalAgg =
AggregateCall.create(
SqlInternalOperators.LITERAL_AGG, false, false, false,
List.of(rexBuilder.makeLiteral(true)), List.of(), -1, null,
RelCollations.EMPTY, typeFactory.createSqlType(SqlTypeName.BOOLEAN), "li");
ImmutableBitSet g0 = ImmutableBitSet.of(0);
ImmutableBitSet g1 = ImmutableBitSet.of(1);
RelNode aggregate =
LogicalAggregate.create(input, List.of(), g0.union(g1), List.of(g0, g1), List.of(literalAgg));
UnsupportedOperationException ex =
assertThrows(
UnsupportedOperationException.class,
() -> SubstraitRelVisitor.convert(RelRoot.of(aggregate, SqlKind.SELECT), EXTENSION_COLLECTION));
assertTrue(ex.getMessage().contains("GROUPING SETS"), ex.getMessage());
}
private static boolean containsLiteralAgg(RelNode node) {
if (node instanceof org.apache.calcite.rel.core.Aggregate
&& ((org.apache.calcite.rel.core.Aggregate) node)
.getAggCallList().stream()
.anyMatch(c -> c.getAggregation().getKind() == SqlKind.LITERAL_AGG)) {
return true;
}
return node.getInputs().stream().anyMatch(OptimizerIntegrationTest::containsLiteralAgg);
}
private static Optional<Project> findProjectOverAggregate(Rel rel) {
if (rel instanceof Project && ((Project) rel).getInput() instanceof Aggregate) {
return Optional.of((Project) rel);
}
return rel.getInputs().stream()
.map(OptimizerIntegrationTest::findProjectOverAggregate)
.filter(Optional::isPresent).map(Optional::get).findFirst();
} |
fix: calcite optimization adds literal_agg
I was testing running various Calcite optimisations; TPC/H 16 specifically was optimised in such a way that it wasn't possible for the Isthmus
SubstraitRelVisitorto handle it.The issue was that calcite had added
LITERAL_AGG(true).In Apache Calcite,
LITERAL_AGG(true)is an internal aggregate function injected during the decorrelation process (specifically when handlingANY,SOME, orINsubqueries).Here is why it is there and what it does:
1. The Problem: Handling Empty Subqueries
In your "before" plan, there is a
<> SOME(...)correlated subquery. By SQL standards, if a subquery inside a quantified comparison (SOME/ANY) returns zero rows, the entire condition evaluates toFALSE(orUNKNOWN), notNULL.2. The Solution: Flagging Matches
When Calcite transforms the correlated subquery into a join (
LogicalCorrelate/LogicalAggregate), it needs a foolproof way to know if the subquery actually produced any rows for a given correlated key ($cor0.L_PARTKEY).LITERAL_AGG(true)evaluates the literal valuetruefor every row entering the aggregate.LITERAL_AGG(true)will returnNULL.true.3. How Calcite Uses It
Look closely at the massive
LogicalFilter(condition=[OR(...)]right above the correlate in your "after" plan.Calcite uses the output of
LITERAL_AGG(true)(which maps to column$19or$20in that flattened row) to evaluate the exact short-circuiting logic of theSOMEoperator:If it's NULL: The subquery was empty$\rightarrow$ handle as false/null.$\rightarrow$ proceed to check the actual value comparisons (like
If it's TRUE: The subquery returned data
COUNTandMAX).It essentially acts as a highly optimized, null-aware boolean flag for row existence during complex subquery unnesting.
🤖 built with assistance from AI