github-actions[bot] commented on code in PR #68787:
URL: https://github.com/apache/doris/pull/68787#discussion_r4219688711
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/ivm/agg/IvmAggExtremalProcessor.java:
##########
@@ -107,7 +107,7 @@ protected List<IvmAggFunctionKind>
zeroDefaultSlotKinds(IvmAggTarget target, boo
@Override
public void appendApplyExpressions(IvmAggTarget target, IvmAggApplyContext
applyContext) {
IvmAggExpressionBuilder ctx = applyContext.expressions();
- Slot oldExtreme =
applyContext.rawMvSlot(target.getVisibleSlot().getName());
+ Slot oldExtreme =
applyContext.rawMvSlot(target.getValueStateColumnName());
Review Comment:
[P2] Cover boundary-delete fallback with a wrapped MIN/MAX carrier. The new
suite only updates or deletes non-boundary values, while
`test_ivm_minmax_runtime_fallback` uses bare MIN/MAX and reads their visible
columns. This changed `oldExtreme` lookup therefore has no test when deleting
the old minimum or maximum must raise `MIN_MAX_BOUNDARY_HIT`. Add a wrapped
view with the `_MIN`/`_MAX` carrier, verify strict incremental leaves it
unchanged, then run `INCREMENTAL FALLBACK` and compare against the source query.
##########
regression-test/suites/mtmv_p0/ivm/test_ivm_agg_expr_over_agg_1.groovy:
##########
@@ -0,0 +1,667 @@
+// 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.
+
+suite("test_ivm_agg_expr_over_agg_1") {
+
+ // =========================================================
+ // A scalar expression wrapped around an aggregate result, as
+ // in SELECT k, SUM(v) * 100 FROM t GROUP BY k, must stay
+ // incrementally maintainable.
+ //
+ // Apply merges the old MV state in the state domain and then
+ // re-applies the outer expression:
+ // new.s100 = f(apply(old_mv.sum_v, delta.sum_v))
+ // so the MV must persist a column carrying SUM(v) itself. That
+ // column is the visible aggregate output when the select list
+ // projects it (SELECT SUM(v) AS s, SUM(v) * 100) and a
+ // materialized hidden column when an upper expression consumes
+ // it without projecting it (SELECT SUM(v) * 100).
+ //
+ // These cases verify both halves of the invariant:
+ // * the hidden layout, via DESC (the dropped state column is
+ // materialized, once per aggregate state, reusing existing
+ // columns when they already carry it);
+ // * the merged values, through INSERT/UPDATE/DELETE and
+ // incremental refreshes.
+ //
+ // NOTE: set show_hidden_columns=true right before a DESC only —
+ // enabling it earlier puts the session in debug mode and blocks
+ // CREATE MATERIALIZED VIEW.
+ // =========================================================
+
+ def refreshIncremental = { mv ->
+ sql """REFRESH MATERIALIZED VIEW ${mv} INCREMENTAL"""
+ waitingMTMVTaskFinishedByMvName(mv)
+ }
+
+ sql """drop materialized view if exists test_ivm_expr_over_agg_sum;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_cnt;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_min;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_max;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_list;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_div;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_cast;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_scalar;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_sum_avg;"""
+ sql """drop materialized view if exists
test_ivm_expr_over_agg_sum_avg_mul;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_plain;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_cnt_star;"""
+ sql """drop materialized view if exists
test_ivm_expr_over_agg_avg_round;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_bitmap;"""
+ sql """drop materialized view if exists
test_ivm_expr_over_agg_bitmap_union;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_agg_arg;"""
+ sql """drop materialized view if exists test_ivm_expr_over_agg_key_expr;"""
+ sql """drop table if exists test_ivm_expr_over_agg_base;"""
+
+ sql """
+ CREATE TABLE test_ivm_expr_over_agg_base (
+ id INT,
+ k INT,
+ v INT
+ )
+ UNIQUE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 2
+ PROPERTIES (
+ "replication_num" = "1",
+ "binlog.enable" = "true",
+ "binlog.format" = "ROW", "binlog.need_historical_value" = "true",
+ "enable_unique_key_merge_on_write" = "true"
+ );
+ """
+
+ // =========================================================
+ // Part 1: hidden layout of the wrapped-aggregate shapes
+ // =========================================================
+
+ // SUM(v) * 100: SUM's own value is its mergeable state and the visible
column is
+ // consumed by the outer expression, so the state is materialized as
_0_SUM_COL__
+ // next to the hidden non-NULL count.
+ sql """
+ CREATE MATERIALIZED VIEW test_ivm_expr_over_agg_sum
+ BUILD DEFERRED REFRESH INCREMENTAL ON MANUAL
+ DISTRIBUTED BY RANDOM BUCKETS 2
+ PROPERTIES ('replication_num' = '1')
+ AS SELECT k, SUM(v) * 100 AS s100 FROM test_ivm_expr_over_agg_base
GROUP BY k;
+ """
+ sql """set show_hidden_columns=true"""
+ qt_sum_desc """DESC test_ivm_expr_over_agg_sum"""
+ sql """set show_hidden_columns=false"""
+
+ // COUNT(v) + 1: same for COUNT(expr), whose visible column is the count
state.
+ sql """
+ CREATE MATERIALIZED VIEW test_ivm_expr_over_agg_cnt
+ BUILD DEFERRED REFRESH INCREMENTAL ON MANUAL
+ DISTRIBUTED BY RANDOM BUCKETS 2
+ PROPERTIES ('replication_num' = '1')
+ AS SELECT k, COUNT(v) + 1 AS c1 FROM test_ivm_expr_over_agg_base GROUP
BY k;
+ """
+ sql """set show_hidden_columns=true"""
+ qt_cnt_desc """DESC test_ivm_expr_over_agg_cnt"""
+ sql """set show_hidden_columns=false"""
+
+ // MIN(v) * 2 and MAX(v) + 1.
+ sql """
+ CREATE MATERIALIZED VIEW test_ivm_expr_over_agg_min
+ BUILD DEFERRED REFRESH INCREMENTAL ON MANUAL
+ DISTRIBUTED BY RANDOM BUCKETS 2
+ PROPERTIES ('replication_num' = '1')
+ AS SELECT k, MIN(v) * 2 AS m2 FROM test_ivm_expr_over_agg_base GROUP
BY k;
+ """
+ sql """set show_hidden_columns=true"""
+ qt_min_desc """DESC test_ivm_expr_over_agg_min"""
+ sql """set show_hidden_columns=false"""
+
+ sql """
+ CREATE MATERIALIZED VIEW test_ivm_expr_over_agg_max
+ BUILD DEFERRED REFRESH INCREMENTAL ON MANUAL
+ DISTRIBUTED BY RANDOM BUCKETS 2
+ PROPERTIES ('replication_num' = '1')
+ AS SELECT k, MAX(v) + 1 AS m1 FROM test_ivm_expr_over_agg_base GROUP
BY k;
+ """
+ sql """set show_hidden_columns=true"""
+ qt_max_desc """DESC test_ivm_expr_over_agg_max"""
+ sql """set show_hidden_columns=false"""
+
+ // ARRAY_SIZE(COLLECT_LIST(v)): the visible array is the whole aggregate
state.
+ sql """
+ CREATE MATERIALIZED VIEW test_ivm_expr_over_agg_list
+ BUILD DEFERRED REFRESH INCREMENTAL ON MANUAL
+ DISTRIBUTED BY RANDOM BUCKETS 2
+ PROPERTIES ('replication_num' = '1')
+ AS SELECT k, ARRAY_SIZE(COLLECT_LIST(v)) AS n FROM
test_ivm_expr_over_agg_base GROUP BY k;
Review Comment:
[P2] Make the wrapped COLLECT_LIST check sensitive to carrier contents. This
view exposes only `ARRAY_SIZE(COLLECT_LIST(v))`; after id=2 changes 20 to 25,
both the correct carrier `[10,25,30]` and a stale `[10,20,30]` still return 3,
and no later step removes 25. Thus every new source comparison can pass while
the persisted state is wrong. Compare a content-sensitive wrapper such as
`ARRAY_SORT(COLLECT_LIST(v))`, or inspect the hidden carrier after the update
and a later delete of 25.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/ivm/agg/IvmAggArrayProcessor.java:
##########
@@ -65,7 +65,7 @@ void mapApplyDeltaSlots(IvmAggTarget target, Map<String,
Slot> outputByName,
@Override
public void appendApplyExpressions(IvmAggTarget target, IvmAggApplyContext
applyContext) {
IvmAggExpressionBuilder ctx = applyContext.expressions();
- Slot oldArray =
applyContext.rawMvSlot(target.getVisibleSlot().getName());
+ Slot oldArray =
applyContext.rawMvSlot(target.getValueStateColumnName());
Review Comment:
[P2] Exercise a wrapped ARRAY_AGG through the new carrier lookup. This read
now uses `getValueStateColumnName()` for both array processors, but the new
suite wraps only `COLLECT_LIST`; the existing ARRAY_AGG suite stores its array
in a visible column. ARRAY_AGG retains NULL elements through its separate
packed-delta path, so neither suite proves that a view such as
`ARRAY_SIZE(ARRAY_AGG(v))` can CREATE, merge a later NULL-bearing update
through `__DORIS_IVM_AGG_0_ARRAY_AGG_COL__`, and COMPLETE refresh. Add that
shape with layout and source-query checks across both refresh modes.
--
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]
