Etienne Pelissier created CALCITE-7687:
------------------------------------------
Summary: RelMdSelectivity and RelMdDistinctRowCount for Aggregate
can propagate a predicate with wrong references
Key: CALCITE-7687
URL: https://issues.apache.org/jira/browse/CALCITE-7687
Project: Calcite
Issue Type: Bug
Components: core
Affects Versions: 1.42.0
Reporter: Etienne Pelissier
This is the same defect as CALCITE-4414, but in the {{Aggregate}} overloads,
which were not swept [when that issue was
fixed|https://github.com/apache/calcite/commit/b4e399cb35224d8c8d55f02b7cf2b9649a3b28a4]
for {{Calc}} in 1.27.0.
An {{Aggregate}} derives its row type as {{{}(group keys..., agg calls...){}}},
so output field {{i}} is input field {{{}groupSet.nth(i){}}}. Two metadata
handlers forward a predicate expressed over the aggregate's *output* to the
aggregate's *input* without applying that translation.
*Minimal repros covering both handlers are in the comments of this ticket.*
h3. 1. [RelMdSelectivity#getSelectivity(Aggregate,
...)|https://github.com/apache/calcite/blob/7939fa2163467205726764fb2e575f8b289c1b8b/core/src/main/java/org/apache/calcite/rel/metadata/RelMdSelectivity.java#L178]
{code:java}
RelOptUtil.splitFilters(rel.getGroupSet(), predicate, pushable, notPushable);
RexNode childPred = RexUtil.composeConjunction(rexBuilder, pushable, true);
// childPred not translated
Double selectivity = mq.getSelectivity(rel.getInput(), childPred);{code}
Two independent problems.
*(a) No translation.* Exactly as in
[CALCITE-4414|https://github.com/apache/calcite/commit/b4e399cb35224d8c8d55f02b7cf2b9649a3b28a4].
Compare {{{}getSelectivity(Project, ...){}}}, which calls
{{RelOptUtil.pushPastProject}} before recursing, and {{{}getSelectivity(Calc,
...){}}}, which calls {{RelOptUtil.pushPastCalc}} since
[CALCITE-4414|https://github.com/apache/calcite/commit/b4e399cb35224d8c8d55f02b7cf2b9649a3b28a4].
*(b) Wrong pushability* *bitmap.* {{predicate}} is in output index space, but
{{rel.getGroupSet() }}holds *input* indices, so {{splitFilters}} compares the
two spaces against one another. The correct bitmap is
{{{}ImmutableBitSet.range(rel.getGroupCount()){}}}, which is what
{{RelMdDistinctRowCount}} already uses for the same purpose, so the two
handlers currently disagree.
h3. 2. [RelMdDistinctRowCount#getDistinctRowCount(Aggregate,
...)|https://github.com/apache/calcite/blob/7939fa2163467205726764fb2e575f8b289c1b8b/core/src/main/java/org/apache/calcite/rel/metadata/RelMdDistinctRowCount.java#L168]
{code:java}
RelOptUtil.splitFilters(
ImmutableBitSet.range(rel.getGroupCount()), predicate, pushable, notPushable);
RexNode childPreds = RexUtil.composeConjunction(rexBuilder, pushable, true);
// set the bits as they correspond to the child input
RelMdUtil.setAggChildKeys(groupKey, rel, childKey);
// childPreds not translated
Double distinctRowCount = mq.getDistinctRowCount(rel.getInput(),
childKey.build(), childPreds); {code}
One problem.
*(a) No translation.*
h3. 3. Symptom
Unlike CALCITE-4414, which threw {{{}ArrayIndexOutOfBoundsException{}}}, this
is silent.
{{splitFilters}} only pushes conjuncts whose refs are inside the bitmap, so the
pushed index is always valid: it just names a different column.
That is harmless while the handler below keys only off {{SqlKind
(RelMdUtil.guessSelectivity)}} , which is why it has gone unnoticed.
h3. 4. Suggested fix
[{{FlinkRelMdUtil.splitPredicateOnAgg}}|https://github.com/apache/flink/blob/12197ea92a5667073bc0c6810e526a39979d835c/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/FlinkRelMdUtil.scala#L556]
already addresses both problems:
{code:java}
RelOptUtil.splitFilters(ImmutableBitSet.range(0, numOfGroupKey), predicate,
pushable, notPushable) val adjustments = new Array[Int](aggOutputFields.size)
grouping.zipWithIndex.foreach { case (bit, index) => adjustments(index) = bit -
index } pushCondition.accept(new RelOptUtil.RexInputConverter( rexBuilder,
aggOutputFields, aggInputFields, adjustments)){code}
{{RexInputConverter}} is already used this way by both {{Union}} handlers. A
{{RelOptUtil.pushPastAggregate}} helper alongside {{pushPastProject}} /
{{pushPastCalc}} would let both handlers share one implementation.
h3. 5. Downstream impact
*Drill* - most exposed:
[{{DrillRelMdSelectivity#getScanSelectivity}}|https://github.com/apache/drill/blob/23bc6619705fe4f625a4dbe68e0044bd8dead73b/exec/java-exec/src/main/java/org/apache/drill/exec/planner/cost/DrillRelMdSelectivity.java#L117]
consults per-column statistics, so this is a wrong estimate rather than a
differently-wrong constant. Neither Drill handler overrides {{{}Aggregate{}}}.
*Hive* - neither handler overrides {{{}Aggregate{}}}.
*Kylin* - {{{}DefaultRelMetadataProvider{}}}, no custom handlers.
*Flink* - unaffected (fix above).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
