[BugFix] window lose tuple.computeMemory (backport StarRocks#41442) (S…

…tarRocks#41673) Signed-off-by: Seaven <seaven_7@qq.com>
Seaven · Feb 27, 2024 · 4d1289c · 4d1289c
1 parent 2d0d90b
commit 4d1289c
Show file tree

Hide file tree

Showing 6 changed files with 48 additions and 4 deletions.
diff --git a/be/src/exec/vectorized/aggregator.cpp b/be/src/exec/vectorized/aggregator.cpp
@@ -790,6 +790,9 @@ Status Aggregator::_evaluate_exprs(vectorized::Chunk* chunk) {
             // TODO: optimized the memory usage
             _group_by_columns[i] = vectorized::NullableColumn::create(
                     _group_by_columns[i], vectorized::NullColumn::create(_group_by_columns[i]->size(), 0));
+        } else if (!_group_by_types[i].is_nullable && _group_by_columns[i]->is_nullable()) {
+            return Status::InternalError(fmt::format("error nullablel column, index: {}, slot: {}", i,
+                                                     _group_by_expr_ctxs[i]->root()->debug_string()));
         }
     }
 

diff --git a/fe/fe-core/src/main/java/com/starrocks/analysis/SlotDescriptor.java b/fe/fe-core/src/main/java/com/starrocks/analysis/SlotDescriptor.java
@@ -23,14 +23,10 @@
 
 import com.google.common.base.MoreObjects;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-import com.starrocks.catalog.ArrayType;
 import com.starrocks.catalog.Column;
 import com.starrocks.catalog.ColumnStats;
-import com.starrocks.catalog.MapType;
 import com.starrocks.catalog.ScalarType;
-import com.starrocks.catalog.StructType;
 import com.starrocks.catalog.Type;
 import com.starrocks.sql.analyzer.SemanticException;
 import com.starrocks.thrift.TSlotDescriptor;
@@ -176,6 +172,12 @@ public boolean getIsNullable() {
 
     public void setIsNullable(boolean value) {
         isNullable = value;
+        // NullIndicatorBit is deprecated in BE, we mock bit to avoid BE crash
+        if (isNullable) {
+            setNullIndicatorBit(0);
+        } else {
+            setNullIndicatorBit(-1);
+        }
     }
 
     public void setByteSize(int byteSize) {

diff --git a/fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java b/fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java
@@ -2387,6 +2387,7 @@ public PlanFragment visitPhysicalAnalytic(OptExpression optExpr, ExecPlan contex
                 context.getColRefToExpr()
                         .put(analyticCall.getKey(), new SlotRef(analyticCall.getKey().toString(), slotDesc));
             }
+            outputTupleDesc.computeMemLayout();
 
             List<Expr> partitionExprs =
                     node.getPartitionExpressions().stream().map(e -> ScalarOperatorToExpr.buildExecExpression(e,

diff --git a/fe/fe-core/src/test/java/com/starrocks/sql/plan/PlanTestNoneDBBase.java b/fe/fe-core/src/test/java/com/starrocks/sql/plan/PlanTestNoneDBBase.java
@@ -159,6 +159,10 @@ public String getThriftPlan(String sql) throws Exception {
         return UtFrameUtils.getPlanThriftString(connectContext, sql);
     }
 
+    public String getDescTbl(String sql) throws Exception {
+        return UtFrameUtils.getThriftDescTbl(connectContext, sql);
+    }
+
     public static int getPlanCount(String sql) throws Exception {
         connectContext.getSessionVariable().setUseNthExecPlan(1);
         int planCount = UtFrameUtils.getPlanAndFragment(connectContext, sql).second.getPlanCount();

diff --git a/fe/fe-core/src/test/java/com/starrocks/sql/plan/WindowTest.java b/fe/fe-core/src/test/java/com/starrocks/sql/plan/WindowTest.java
@@ -812,4 +812,33 @@ public void testWindowColumnReuse() throws Exception {
                 "args nullable: true; result nullable: true]\n" +
                 "  |  cardinality: 1");
     }
+
+    @Test
+    public void testWindowOutputColumnNullCheck() throws Exception {
+        String sql = "select t1a, t1b, t1c, count(t1d) over (partition by t1d) " +
+                "from test_all_type_not_null";
+        String plan = getVerboseExplain(sql);
+        assertContains(plan, "  2:ANALYTIC\n" +
+                "  |  functions: [, count[([4: t1d, BIGINT, false]); args: BIGINT; result: BIGINT; " +
+                "args nullable: false; result nullable: false], ]\n" +
+                "  |  partition by: [4: t1d, BIGINT, false]\n" +
+                "  |  cardinality: 1");
+        assertContains(plan, "  3:Project\n" +
+                "  |  output columns:\n" +
+                "  |  1 <-> [1: t1a, VARCHAR, false]\n" +
+                "  |  2 <-> [2: t1b, SMALLINT, false]\n" +
+                "  |  3 <-> [3: t1c, INT, false]\n" +
+                "  |  11 <-> [11: count(4: t1d), BIGINT, false]\n" +
+                "  |  cardinality: 1");
+
+        plan = getDescTbl(sql);
+        assertContains(plan, "TSlotDescriptor(id:11, parent:2, " +
+                "slotType:TTypeDesc(types:[TTypeNode(type:SCALAR, scalar_type:TScalarType(type:BIGINT))]), " +
+                "columnPos:-1, byteOffset:0, nullIndicatorByte:0, nullIndicatorBit:-1, " +
+                "colName:, slotIdx:0, isMaterialized:true)");
+        assertContains(plan, "TSlotDescriptor(id:11, parent:4, " +
+                "slotType:TTypeDesc(types:[TTypeNode(type:SCALAR, scalar_type:TScalarType(type:BIGINT))]), " +
+                "columnPos:-1, byteOffset:8, nullIndicatorByte:0, nullIndicatorBit:-1, " +
+                "colName:, slotIdx:2, isMaterialized:true)");
+    }
 }
diff --git a/fe/fe-core/src/test/java/com/starrocks/utframe/UtFrameUtils.java b/fe/fe-core/src/test/java/com/starrocks/utframe/UtFrameUtils.java
@@ -654,6 +654,11 @@ public static String getPlanThriftString(ConnectContext ctx, String queryStr) th
         return UtFrameUtils.getThriftString(UtFrameUtils.getPlanAndFragment(ctx, queryStr).second.getFragments());
     }
 
+    public static String getThriftDescTbl(ConnectContext ctx, String queryStr) throws Exception {
+        Pair<String, ExecPlan> pair = UtFrameUtils.getPlanAndFragment(ctx, queryStr);
+        return pair.second.getDescTbl().toThrift().toString();
+    }
+
     // Lock all database before analyze
     private static void lock(Map<String, Database> dbs) {
         if (dbs == null) {