github-actions[bot] commented on code in PR #69012:
URL: https://github.com/apache/doris/pull/69012#discussion_r4256422662
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/types/AggStateType.java:
##########
@@ -93,6 +94,14 @@ public AggStateType(String functionName, List<DataType>
subTypes,
this.looseTypeCoercionAllowed = looseTypeCoercionAllowed;
}
+ /** Resolve SQL type declarations without changing the decoder for
persisted states. */
+ public static AggStateType fromSql(String functionName, List<DataType>
subTypes,
+ List<Boolean> subTypeNullables, boolean returnNullable) {
+ String resolvedName = "bitmap_intersect".equalsIgnoreCase(functionName)
Review Comment:
[P2] Preserve legacy state identity across schema SQL round trips
Legacy metadata, including a newly declared
`AGG_STATE<bitmap_intersect_v1(bitmap NULL)>`, is canonicalized to
`bitmap_intersect`, and catalog `AggStateType.toSql()` still prints that name.
`CREATE TABLE clone LIKE legacy` renders the source schema through
`Env.getCreateTableLikeStmt()` and reparses it in `CreateTableLikeCommand`, so
this remapping silently makes the clone's column v2. `INSERT INTO clone SELECT
* FROM legacy` then fails the distinct AGG_STATE type checks instead of copying
the same schema; replaying SHOW CREATE has the same problem. Emit the explicit
v1 spelling for legacy schema SQL, or preserve the canonical decoder during
internal schema reconstruction, and cover the SQL/LIKE round trip.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/BuiltinAggregateFunctions.java:
##########
@@ -136,7 +137,8 @@ private BuiltinAggregateFunctions() {
agg(AvgMap.class, "avg_map"),
agg(AvgWeighted.class, "avg_weighted"),
agg(BitmapAgg.class, "bitmap_agg"),
- agg(BitmapIntersect.class, "bitmap_intersect"),
+ agg(BitmapIntersect.class, "bitmap_intersect_v1"),
+ agg(BitmapIntersectV2.class, "bitmap_intersect_v2",
"bitmap_intersect"),
Review Comment:
[P1] Keep internally generated legacy unions bound to v1
For a legacy (or explicit `bitmap_intersect_v1`) GENERIC column in a
RANDOM-distributed aggregate table,
`BindRelation.preAggForRandomDistribution()` derives `bitmap_intersect_union`
from the persisted function name. This registration now builds
`BitmapIntersectV2`, so a simple read is rewritten as:
```text
Project(state)
Aggregate(id, bitmap_intersect_v2_union(state:v1))
Scan(legacy RANDOM aggregate table)
```
`ExpressionTranslator` sends that v2 name with the unchanged v1 state type,
and `AggFnEvaluator::prepare()` rejects it with `not match function
bitmap_intersect_v2_union`. Even explicitly using `bitmap_intersect_v1_merge`
cannot avoid the implicit union below it. Internal canonical-name lookups (also
used by combinator MV rollups) need to preserve the stored state's v1 identity
while the public SQL alias selects v2.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/expressions/functions/agg/BitmapIntersectV2.java:
##########
@@ -0,0 +1,53 @@
+// 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.
+
+package org.apache.doris.nereids.trees.expressions.functions.agg;
+
+import org.apache.doris.nereids.trees.expressions.Expression;
+import org.apache.doris.nereids.types.DataType;
+import org.apache.doris.nereids.types.StringType;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
+
+import java.util.List;
+
+/** BITMAP_INTERSECT with an intermediate state that preserves initialization.
*/
+public class BitmapIntersectV2 extends BitmapIntersect {
+ public BitmapIntersectV2(Expression arg) {
+ super("bitmap_intersect_v2", arg);
Review Comment:
[P1] Gate the public v2 binding during rolling upgrades
The public `bitmap_intersect` alias now always constructs this expression,
and `ExpressionTranslator` sends `bitmap_intersect_v2` with a STRING
intermediate type. A backend from before this PR registers only
`bitmap_intersect`, so `SELECT bitmap_intersect(b) FROM t` fails with `Agg
Function ... is not implemented` whenever a fragment executes on a backend that
has not yet been upgraded. The merge-base and this head both support execution
version 16; lowering `be_exec_version` does not change the unconditional FE
binding, and no bitmap version alternative is registered. Preserve the legacy
symbol and BITMAP intermediate type until the executing backends support v2,
with the same decision applied to state declarations and all aggregation phases.
##########
regression-test/suites/bitmap_functions/test_bitmap_intersect_null_partial.groovy:
##########
@@ -0,0 +1,123 @@
+// 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_bitmap_intersect_null_partial", "p0") {
+ sql "SET enable_bucketed_hash_agg=false"
+ sql "SET parallel_pipeline_task_num=4"
+ sql "DROP TABLE IF EXISTS test_bitmap_intersect_null_partial"
+ sql """
+ CREATE TABLE test_bitmap_intersect_null_partial (
+ id INT, g INT, s STRING NULL
+ ) DUPLICATE KEY(id)
+ DISTRIBUTED BY HASH(id) BUCKETS 4
+ PROPERTIES('replication_num'='1')
+ """
+ sql """
+ INSERT INTO test_bitmap_intersect_null_partial VALUES
+ (1,1,'1,2'), (2,1,NULL), (3,1,'2,3'),
+ (4,2,NULL), (5,2,NULL),
+ (6,3,'1,2'), (7,3,''), (8,3,NULL),
+ (9,4,'1'), (10,4,'2'), (11,4,NULL)
+ """
+
+ for (def nullV2 : [false, true]) {
+ sql "SET enable_aggregate_function_null_v2=${nullV2}"
+ for (def phase : [1, 2]) {
+ sql "SET agg_phase=${phase}"
+ order_qt_grouped """
+ SELECT g,
bitmap_to_string(bitmap_intersect(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial GROUP BY g
+ """
+ order_qt_filtered """
+ SELECT g,
bitmap_to_string(bitmap_intersect(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial WHERE s IS NOT NULL
GROUP BY g
+ """
+ order_qt_without_key """
+ SELECT
bitmap_to_string(bitmap_intersect(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial WHERE g=1
+ """
+ order_qt_null_only """
+ SELECT
bitmap_to_string(bitmap_intersect(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial WHERE g=2
+ """
+ order_qt_empty_input """
+ SELECT
bitmap_to_string(bitmap_intersect(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial WHERE id<0
+ """
+ order_qt_union_control """
+ SELECT bitmap_to_string(bitmap_union(bitmap_from_string(s)))
+ FROM test_bitmap_intersect_null_partial WHERE g=1
+ """
+ order_qt_state_merge """
+ SELECT bitmap_to_string(bitmap_intersect_merge(state))
+ FROM (
+ SELECT bitmap_intersect_state(bitmap_from_string(s)) AS
state
+ FROM test_bitmap_intersect_null_partial WHERE g=1
+ ) states
+ """
+ }
+ }
+
+ sql "DROP TABLE IF EXISTS test_bitmap_intersect_partial_state"
+ sql """
+ CREATE TABLE test_bitmap_intersect_partial_state (
+ id INT, state AGG_STATE<bitmap_intersect(bitmap NULL)> GENERIC
Review Comment:
[P2] Enable AGG_STATE before creating the state tables
The suite never sets `enable_agg_state=true`, while
`SessionVariable.enableAggState` defaults to false. This first GENERIC column
therefore reaches `ColumnDefinition.validate()`'s `agg state not enable, need
set enable_agg_state=true` error when the suite runs on a default cluster. It
aborts before either stored-state result or the legacy/mismatched-format
checks. Add the session setting before creating these tables, as the existing
bitmap AGG_STATE suite does, so this regression is independently runnable.
--
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]
