Skip to content

Commit 8a725a4

Browse files
fix ut
Signed-off-by: silverbullet233 <3675229+silverbullet233@users.noreply.github.com>
1 parent 3b904ed commit 8a725a4

File tree

8 files changed

+44
-26
lines changed

8 files changed

+44
-26
lines changed

be/src/exec/hash_joiner.cpp

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -528,12 +528,8 @@ Status HashJoiner::_process_other_conjunct(ChunkPtr* chunk, JoinHashTable& hash_
528528

529529
Status HashJoiner::_process_where_conjunct(ChunkPtr* chunk) {
530530
SCOPED_TIMER(probe_metrics().where_conjunct_evaluate_timer);
531-
if (!_common_expr_ctxs.empty()) {
532-
for (auto& [slot_id, ctx] : _common_expr_ctxs) {
533-
ASSIGN_OR_RETURN(ColumnPtr column, ctx->evaluate((*chunk).get()))
534-
(*chunk)->append_column(column, slot_id);
535-
}
536-
}
531+
CommonExprEvalScopeGuard guard(*chunk, _common_expr_ctxs);
532+
RETURN_IF_ERROR(guard.evaluate());
537533
return ExecNode::eval_conjuncts(_conjunct_ctxs, (*chunk).get());
538534
}
539535

be/src/exec/pipeline/nljoin/nljoin_probe_operator.cpp

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -298,13 +298,6 @@ void NLJoinProbeOperator::iterate_enumerate_chunk(const ChunkPtr& chunk,
298298
call(false, 0, chunk->num_rows());
299299
}
300300
}
301-
Status NLJoinProbeOperator::_eval_common_exprs(const ChunkPtr& chunk) {
302-
for (const auto& [slot_id, ctx] : _common_expr_ctxs) {
303-
ASSIGN_OR_RETURN(ColumnPtr column, ctx->evaluate(chunk.get()));
304-
chunk->append_column(std::move(column), slot_id);
305-
}
306-
return Status::OK();
307-
}
308301

309302
Status NLJoinProbeOperator::_eval_nullaware_anti_conjuncts(const ChunkPtr& chunk, FilterPtr* filter) {
310303
if (!_join_conjuncts.empty() && chunk && !chunk->is_empty()) {

be/src/exec/pipeline/nljoin/nljoin_probe_operator.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,6 @@ class NLJoinProbeOperator final : public OperatorWithDependency {
106106
bool _is_left_semi_join() const;
107107
bool _is_left_anti_join() const;
108108
bool _is_null_aware_left_anti_join() const;
109-
Status _eval_common_exprs(const ChunkPtr& chunk);
110109

111110
private:
112111
const TJoinOp::type _join_op;

be/src/storage/chunk_helper.h

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -222,6 +222,18 @@ class SegmentedChunk final : public std::enable_shared_from_this<SegmentedChunk>
222222
};
223223

224224
class ExprContext;
225+
/**
226+
* RAII guard for evaluating common expressions on a chunk.
227+
*
228+
* This class provides automatic scope management for evaluating common expressions
229+
* that are temporarily used during expression computation. Common expressions are
230+
* computed once and reused across multiple expressions to avoid redundant computation,
231+
* but they are only needed during the computation phase and should be cleaned up
232+
* from the chunk after computation completes.
233+
*
234+
* The destructor automatically removes the common expressions from the chunk
235+
* to prevent memory leaks and ensure proper cleanup.
236+
*/
225237
class CommonExprEvalScopeGuard {
226238
public:
227239
CommonExprEvalScopeGuard(const ChunkPtr& chunk, const std::map<SlotId, ExprContext*>& common_expr_ctxs);

fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3712,8 +3712,11 @@ private JoinExprInfo buildJoinExpr(OptExpression optExpr, ExecPlan context) {
37123712
List<Expr> otherJoinConjuncts = otherJoin.stream().map(e -> ScalarOperatorToExpr.buildExecExpression(e,
37133713
new ScalarOperatorToExpr.FormatterContext(context.getColRefToExpr())))
37143714
.collect(Collectors.toList());
3715-
PhysicalJoinOperator joinOperator = (PhysicalJoinOperator) optExpr.getOp();
3716-
Map<SlotId, Expr> commonSubExprMap = buildCommonSubExprMap(joinOperator.getPredicateCommonOperators(), context);
3715+
Map<SlotId, Expr> commonSubExprMap = Maps.newHashMap();
3716+
if (optExpr.getOp() instanceof PhysicalJoinOperator) {
3717+
PhysicalJoinOperator joinOperator = (PhysicalJoinOperator) optExpr.getOp();
3718+
commonSubExprMap = buildCommonSubExprMap(joinOperator.getPredicateCommonOperators(), context);
3719+
}
37173720

37183721
List<ScalarOperator> predicates = Utils.extractConjuncts(predicate);
37193722
List<Expr> conjuncts = predicates.stream().map(e -> ScalarOperatorToExpr.buildExecExpression(e,

fe/fe-core/src/test/java/com/starrocks/planner/PushDownSubfieldHashJoinTest.java

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
import com.starrocks.common.FeConstants;
1818
import com.starrocks.qe.ConnectContext;
19+
import com.starrocks.sql.plan.PlanTestBase;
20+
import com.starrocks.sql.plan.PlanTestNoneDBBase;
1921
import com.starrocks.statistic.StatsConstants;
2022
import com.starrocks.utframe.StarRocksAssert;
2123
import com.starrocks.utframe.UtFrameUtils;
@@ -111,9 +113,15 @@ public void test() throws Exception {
111113
" | colocate: false, reason: \n" +
112114
" | equal join conjunct: 3: fk = 1: fk\n" +
113115
" | other predicates: CAST(array_sum(array_map(<slot 8> -> <slot 8> != 'A', " +
114-
"if(array_length(array_filter(['A','B'], CAST([0,CAST((2: col_int = 1) AND " +
115-
"(4: id IS NOT NULL) AS TINYINT)] AS ARRAY<BOOLEAN>))) = 0, ['C'], " +
116-
"array_filter(['A','B'], CAST([0,CAST((2: col_int = 1) AND (4: id IS NOT NULL) AS TINYINT)] " +
117-
"AS ARRAY<BOOLEAN>))))) AS BOOLEAN)"));
116+
"if(array_length(26: array_filter) = 0, ['C'], 26: array_filter))) AS BOOLEAN)\n" +
117+
" | common sub expr:\n" +
118+
" | <slot 20> : 2: col_int = 1\n" +
119+
" | <slot 21> : 4: id IS NOT NULL\n" +
120+
" | <slot 22> : (20: expr) AND (21: expr)\n" +
121+
" | <slot 23> : CAST(22: expr AS TINYINT)\n" +
122+
" | <slot 24> : [0,CAST((2: col_int = 1) AND (4: id IS NOT NULL) AS TINYINT)]\n" +
123+
" | <slot 25> : CAST([0,CAST((2: col_int = 1) AND (4: id IS NOT NULL) AS TINYINT)] AS ARRAY<BOOLEAN>)\n" +
124+
" | <slot 26> : array_filter(['A','B'], 25: cast)"));
125+
118126
}
119127
}

fe/fe-core/src/test/java/com/starrocks/sql/plan/LowCardinalityTest.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -811,10 +811,11 @@ public void testProject() throws Exception {
811811
// test input two string column
812812
sql = "select if(S_ADDRESS='kks', S_COMMENT, S_COMMENT) from supplier";
813813
plan = getVerboseExplain(sql);
814-
Assertions.assertTrue(plan.contains(
815-
"9 <-> if[(DictDecode(10: S_ADDRESS, [<place-holder> = 'kks']), DictDecode(11: S_COMMENT, [<place-holder>]), " +
816-
"DictDecode(11: S_COMMENT, [<place-holder>])); args: BOOLEAN,VARCHAR,VARCHAR; " +
817-
"result: VARCHAR; args nullable: true; result nullable: true]"));
814+
assertContains(plan, " | 9 <-> if[(DictDecode(10: S_ADDRESS, [<place-holder> = 'kks']), " +
815+
"[12: expr, VARCHAR(101), true], [12: expr, VARCHAR(101), true]); " +
816+
"args: BOOLEAN,VARCHAR,VARCHAR; result: VARCHAR; args nullable: true; result nullable: true]\n" +
817+
" | common expressions:\n" +
818+
" | 12 <-> DictDecode(11: S_COMMENT, [<place-holder>])");
818819
assertNotContains(plan, "DecodeNode");
819820

820821
// common expression reuse 3
@@ -825,7 +826,10 @@ public void testProject() throws Exception {
825826
// support(support(unsupport(Column), unsupport(Column)))
826827
sql = "select REVERSE(SUBSTR(LEFT(REVERSE(S_ADDRESS),INSTR(REVERSE(S_ADDRESS),'/')-1),5)) FROM supplier";
827828
plan = getFragmentPlan(sql);
828-
assertContains(plan, "<slot 9> : reverse(substr(left(DictDecode(10: S_ADDRESS, [reverse(<place-holder>)])");
829+
assertContains(plan, " | <slot 9> : " +
830+
"reverse(substr(left(11: expr, CAST(CAST(instr(11: expr, '/') AS BIGINT) - 1 AS INT)), 5))\n" +
831+
" | common expressions:\n" +
832+
" | <slot 11> : DictDecode(10: S_ADDRESS, [reverse(<place-holder>)])");
829833
}
830834

831835
@Test

fe/fe-core/src/test/java/com/starrocks/sql/plan/LowCardinalityTest2.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -923,7 +923,10 @@ public void testProject() throws Exception {
923923
// support(support(unsupport(Column), unsupport(Column)))
924924
sql = "select REVERSE(SUBSTR(LEFT(REVERSE(S_ADDRESS),INSTR(REVERSE(S_ADDRESS),'/')-1),5)) FROM supplier";
925925
plan = getFragmentPlan(sql);
926-
assertContains(plan, "<slot 9> : reverse(substr(left(DictDecode(10: S_ADDRESS, [reverse(<place-holder>)])");
926+
assertContains(plan, " | <slot 9> : reverse(substr(left(11: expr, " +
927+
"CAST(CAST(instr(11: expr, '/') AS BIGINT) - 1 AS INT)), 5))\n" +
928+
" | common expressions:\n" +
929+
" | <slot 11> : DictDecode(10: S_ADDRESS, [reverse(<place-holder>)])");
927930
}
928931

929932
@Test

0 commit comments

Comments
 (0)