[
https://issues.apache.org/jira/browse/CALCITE-7687?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18101616#comment-18101616
]
Alessandro Solimando commented on CALCITE-7687:
-----------------------------------------------
Hi [~etiennep], thanks for the ticket, from a cursory look this seems to be a
real bug.
It would help if you could summarize the AI-generated output into a better
quality ticket which is well-formatted, less verbose, and generally human
readable (it's literally hard to parse at the moment because rendering is
broken).
If we can get enough information from the ticket, it will be easier to agree on
the direction and then go check the PR.
I haven't checked the associated PR yet as we generally require to have the
Jira ticket in good shape, but comments tend to have similar problems (LLMs
tend to be verbose, describe the migration of the fix instead of the existing
code, use weird vocabulary and jargon, etc.), so in preparation you could do a
pass there too, if needed.
> 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
> Assignee: Etienne Pelissier
> Priority: Major
> Labels: in-progress, 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)
