github-actions[bot] commented on code in PR #69890: URL: https://github.com/apache/doris/pull/69890#discussion_r4425175422
########## regression-test/suites/mtmv_p0/ivm/test_ivm_agg_minmax_topk_1.groovy: ########## @@ -0,0 +1,128 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + + +// Core top-K state transitions with an explicit capacity: truncation, deleting duplicate and last +// extremes, deleting above the bound, exhausting the known range, and changing K with ALTER. +// `SELECT *` with hidden columns prints the state struct of every step; the sequence column differs +// between cloud and shared-nothing deployments and is masked. +suite("test_ivm_agg_minmax_topk_1") { + sql """set enable_materialized_view_rewrite = false;""" + sql """set enable_dml_materialized_view_rewrite = false;""" + sql """drop materialized view if exists ivm_topk_1_mv""" + sql """drop table if exists ivm_topk_1_t""" + + sql """ + CREATE TABLE ivm_topk_1_t ( + id BIGINT NOT NULL, + category VARCHAR(16) NOT NULL, + score BIGINT + ) + UNIQUE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "true", + "changelog.enable" = "true", + "changelog.need_historical_value" = "true" + ) + """ + sql """INSERT INTO ivm_topk_1_t VALUES + (1, 'a', 10), (7, 'a', 10), (2, 'a', 20), (3, 'a', 30), (4, 'a', 40)""" + sql """ + CREATE MATERIALIZED VIEW ivm_topk_1_mv + BUILD DEFERRED REFRESH INCREMENTAL FALLBACK ON MANUAL + KEY(category) + DISTRIBUTED BY HASH(category) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "ivm_minmax_top_k" = "2" + ) + AS SELECT category, MIN(score) AS min_score, MAX(score) AS max_score + FROM ivm_topk_1_t + GROUP BY category + """ + + // The resolved capacity is persisted at CREATE. Assert the one property instead of pinning the + // whole DDL, whose unrelated defaults would make the golden churn. + def createDdl = sql """SHOW CREATE MATERIALIZED VIEW ivm_topk_1_mv""" + assertTrue(createDdl.toString().contains('"ivm_minmax_top_k" = "2"')) Review Comment: [P2] Record deterministic top-K expectations through generated regression output The new suite uses `assertTrue` for SHOW CREATE and `assertEquals` for terminal status/fallback fields, and the same pattern is repeated in topk_2 through topk_7 and the ARRAY case. The regression contract in AGENTS.md requires determined expected results to be expressed through qt/order_qt methods and auto-generated `.out` sections; Groovy assertions leave these checks outside the golden output and make the committed results incomplete. Emit the DDL/status checks through ordered query tags (or another generated result section) and regenerate the affected .out files. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/processor/post/CommonSubExpressionCollector.java: ########## @@ -80,19 +81,64 @@ private int collectChildrenDepth(List<Expression> children, Boolean inLambda) { private int collectCommonExpressionByDepth(int depth, Expression expr, boolean inLambda) { Set<Expression> expressions = getExpressionsFromDepthMap(depth, expressionsByDepth); - // ArrayItemSlot and ArrayItemReference could not be common expressions - // TODO: could not extract common expression when expression contains same lambda expression - // because ArrayItemSlot in Lambda are not same. + // An expression that mentions a lambda argument bound outside it cannot be evaluated + // outside that lambda: hoisting it into an earlier projection leaves the argument without + // its binding. The check must not depend on the inLambda flag -- a lambda nested in another + // expression can be visited again from a context that does not carry the flag (for example + // when the same subtree appears in two projections), and the extraction then breaks the + // lambda scope. An expression that *contains* the whole lambda, such as an array_map with + // its lambda, binds its own arguments and stays extractable. + // A lambda and its parameter references are not standalone values: only an expression that + // contains the whole binding can be hoisted, never the binding pieces themselves. Review Comment: [P1] Keep captured lambda inputs inside their binding scope For `Project(array_map(z -> array_map(y -> y, z), arr))`, `Lambda` stores each parameter as an `ArrayItemReference` whose `getArrayExpression()` is the input array. The inner body only contains the bound `y`; the outer `z` is reachable through the skipped ArrayItemReference child. The new `hasFreeArrayItem` walk therefore returns false and allows CSE to hoist the inner ArrayMap into an outer project, where `z` has no binding (or is replaced by an unrelated slot), changing the result or making the plan invalid. Traverse each ArrayItemReference's source expression under the current bound set, and add a nested captured-input regression. ########## fe/fe-core/src/main/java/org/apache/doris/mtmv/ivm/IvmLinearDeltaHandler.java: ########## @@ -119,6 +119,19 @@ Optional<IvmDeltaRewriteResult> rewriteProject(LogicalProject<? extends Plan> pr childOutputMapping.put(project.child().getOutput().get(i), child.plan.getOutput().get(i)); } for (NamedExpression output : project.getProjects()) { + // A hidden state the child computes itself wins over a project-level derivation of it: + // above the aggregate, normalize derives the MIN/MAX bound from the array for a full + // refresh, while the apply result is the bound the merge computed. Only the states the + // child reports as its own are replaced; everything else (row_id, other hidden + // expressions) keeps the normal rewrite. + if (child.ownedHiddenStateNames.contains(output.getName())) { Review Comment: [P2] Preserve owned MIN/MAX state markers through project rewrites `rewriteAggregate` marks derived state names such as `__DORIS_IVM_AGG_0_MAX_TOPK_BOUND_COL__` as owned so the apply-computed bound is passed through. The project above every normalized aggregate reaches this branch, but the returned `IvmDeltaRewriteResult` at line 148 is built with the legacy constructor and therefore resets `ownedHiddenStateNames` to an empty set (the pass-through wrappers do the same). After an incremental merge that produces exactly K values from complete old/delta sides, apply deliberately writes a NULL bound, but the normalized project recomputes the expression with `cardinality >= K` and writes the Kth value back. A later insert below that false bound is omitted from the state; deleting the current extreme then falsely raises `MIN_MAX_BOUNDARY_HIT`. Carry the owned-name set through every result wrapper and add an exactly-K incremental-bound regression. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
