[
https://issues.apache.org/jira/browse/CALCITE-7687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101409#comment-18101409
]
Etienne Pelissier edited comment on CALCITE-7687 at 8/3/26 2:59 PM:
--------------------------------------------------------------------
Test, for {{getDistinctRowCount}}:
{code:java}
// Same fixture as above. Selectivity.Handler and DistinctRowCount.Handler
both
// declare getDef(), so one class cannot implement both.
private static class DistinctRowCountByColumnTable extends AbstractTable
implements BuiltInMetadata.DistinctRowCount.Handler {
@Nullable RexNode receivedPredicate;
@Nullable ImmutableBitSet receivedGroupKey;
@Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
return abcRowType(typeFactory);
}
@Override public @Nullable Double getDistinctRowCount(RelNode r,
RelMetadataQuery mq, ImmutableBitSet groupKey,
@Nullable RexNode predicate) {
receivedGroupKey = groupKey;
receivedPredicate = predicate;
return 1000.0;
}
}
// The group key is converted into input space by RelMdUtil#setAggChildKeys
and
// arrives as {1}; the predicate beside it is not converted. This handler's
// pushability bitmap is already range(getGroupCount()), so it isolates the
// missing conversion on its own.
@Test void testDistinctRowCountAggregateConvertsPredicateToInputFields() {
final DistinctRowCountByColumnTable table = new
DistinctRowCountByColumnTable();
final RelNode agg = aggregateGroupingOnFields1And2(table);
final RelMetadataQuery mq = agg.getCluster().getMetadataQuery();
// Group key is output $0 ("b", input $1); predicate is on output $1 ("c",
input $2).
mq.getDistinctRowCount(agg, ImmutableBitSet.of(0), isNullOn(agg, 1));
assertThat(table.receivedGroupKey, hasToString("{1}"));
assertThat(table.receivedPredicate, hasToString("IS NULL($2)"));
}
{code}
Fails with:
{noformat}
java.lang.AssertionError:
Expected: with toString() "IS NULL($2)"
but: toString() was "IS NULL($1)"
at
org.apache.calcite.test.RelMetadataTest.testDistinctRowCountAggregateConvertsPredicateToInputFields(RelMetadataTest.java:1899)
{noformat}
was (Author: JIRAUSER313108):
Test, for {{getDistinctRowCount}}:
{code:java}
// Same fixture as above. Selectivity.Handler and DistinctRowCount.Handler
both
// declare getDef(), so one class cannot implement both.
private static class DistinctRowCountByColumnTable extends AbstractTable
implements BuiltInMetadata.DistinctRowCount.Handler {
@Nullable RexNode receivedPredicate;
@Nullable ImmutableBitSet receivedGroupKey;
@Override public RelDataType getRowType(RelDataTypeFactory typeFactory) {
return abcRowType(typeFactory);
}
@Override public @Nullable Double getDistinctRowCount(RelNode r,
RelMetadataQuery mq, ImmutableBitSet groupKey,
@Nullable RexNode predicate) {
receivedGroupKey = groupKey;
receivedPredicate = predicate;
return 1000.0;
}
}
// The group key is converted into input space by RelMdUtil#setAggChildKeys
and
// arrives as {1}; the predicate beside it is not converted. This handler's
// pushability bitmap is already range(getGroupCount()), so it isolates the
// missing conversion on its own.
@Test void testDistinctRowCountAggregateConvertsPredicateToInputFields() {
final DistinctRowCountByColumnTable table = new
DistinctRowCountByColumnTable();
final RelNode agg = aggregateGroupingOnFields1And2(table);
final RelMetadataQuery mq = agg.getCluster().getMetadataQuery();
// Group key is output $0 ("b", input $1); predicate is on output $1 ("c",
input $2).
mq.getDistinctRowCount(agg, ImmutableBitSet.of(0), isNullOn(agg, 1));
assertThat(table.receivedGroupKey, hasToString("{1}"));
assertThat(table.receivedPredicate, hasToString("IS NULL($2)"));
}
{code}
Fails with:
{noformat}
java.lang.AssertionError:
Expected: with toString() "IS NULL($2)"
but: toString() was "IS NULL($1)"
at
org.apache.calcite.test.RelMetadataTest.testDistinctRowCountAggregateConvertsPredicateToInputFields(RelMetadataTest.java:1899)
{noformat}
> 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
> Priority: Major
> Labels: pull-request-available
>
> 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.
> It becomes a *wrong estimate* for any table supplying a
> {{BuiltInMetadata.Selectivity.Handler}} through {{{}RelOptTable.unwrap{}}}.
> 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)
