github-actions[bot] commented on code in PR #59591:
URL: https://github.com/apache/doris/pull/59591#discussion_r2893524740
##########
be/src/pipeline/exec/join/process_hash_table_probe_impl.h:
##########
@@ -165,7 +172,12 @@ void
ProcessHashTableProbe<JoinOpType>::probe_side_output_column(vectorized::Mut
}
}
- if (_left_output_slot_flags[i] &&
!_parent_operator->is_lazy_materialized_column(i)) {
+ // For ASOF JOIN optimized path, skip lazy materialization check
+ constexpr bool is_asof_join = (JoinOpType ==
TJoinOp::ASOF_LEFT_INNER_JOIN ||
Review Comment:
**[Medium]** `DORIS_CHECK(st.ok())` will crash the BE process if the
expression execution fails. While AGENTS.md permits `DORIS_CHECK` for
precondition assertions, an expression execution failure is a runtime error
(e.g., could be caused by malformed data or unsupported type), not an invariant
violation.
Since `_find_batch_asof_optimized` returns `uint32_t` and cannot propagate
`Status`, consider either:
1. Changing the return type to `Status` (or `StatusOr<uint32_t>`) so errors
can propagate to the caller, or
2. If crashing is truly the intended behavior here (because reaching this
point with an invalid expression is considered a logic bug), add a comment
explaining why this is an invariant rather than a runtime error.
Same concern applies to line 35 (`DORIS_CHECK(asof_probe_expr)`) and line 41
(`DORIS_CHECK(probe_col_idx >= 0 ...)`), though those are more defensible as
true precondition assertions.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/NestedLoopJoinNode.java:
##########
@@ -54,7 +54,9 @@ public class NestedLoopJoinNode extends JoinNodeBase {
public static boolean canParallelize(JoinOperator joinOp) {
return joinOp == JoinOperator.CROSS_JOIN || joinOp ==
JoinOperator.INNER_JOIN
|| joinOp == JoinOperator.LEFT_OUTER_JOIN || joinOp ==
JoinOperator.LEFT_SEMI_JOIN
- || joinOp == JoinOperator.LEFT_ANTI_JOIN || joinOp ==
JoinOperator.NULL_AWARE_LEFT_ANTI_JOIN;
+ || joinOp == JoinOperator.LEFT_ANTI_JOIN || joinOp ==
JoinOperator.NULL_AWARE_LEFT_ANTI_JOIN
+ || joinOp == JoinOperator.ASOF_LEFT_INNER_JOIN || joinOp ==
JoinOperator.ASOF_RIGHT_INNER_JOIN
+ || joinOp == JoinOperator.ASOF_LEFT_OUTER_JOIN;
Review Comment:
**[Minor]** `ASOF_RIGHT_OUTER_JOIN` is missing from `canParallelize()`. The
other three ASOF variants (`ASOF_LEFT_INNER_JOIN`, `ASOF_RIGHT_INNER_JOIN`,
`ASOF_LEFT_OUTER_JOIN`) are all included.
If this is intentional (e.g., RIGHT_OUTER cannot be parallelized in NLOJ for
correctness reasons), please add a comment explaining why. If it's an
oversight, it should be added here for consistency.
Note: This may have no practical impact if ASOF joins always use HashJoin
rather than NestedLoopJoin, but it's worth clarifying for maintainability.
--
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]
