github-actions[bot] commented on code in PR #59591:
URL: https://github.com/apache/doris/pull/59591#discussion_r2896615336
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/ReorderJoin.java:
##########
@@ -85,7 +85,8 @@ public class ReorderJoin extends OneRewriteRuleFactory {
public Rule build() {
return logicalFilter(subTree(LogicalJoin.class, LogicalFilter.class))
.whenNot(filter -> filter.child() instanceof LogicalJoin
- && ((LogicalJoin<?, ?>) filter.child()).isMarkJoin())
+ && (((LogicalJoin<?, ?>) filter.child()).isMarkJoin())
+ || ((LogicalJoin<?, ?>)
filter.child()).getJoinType().isAsofJoin())
Review Comment:
**Bug: Operator precedence issue — `||` is outside the `&&` scope.**
In Java, `&&` has higher precedence than `||`. The current code parses as:
```java
(filter.child() instanceof LogicalJoin && isMarkJoin()) || isAsofJoin()
```
This means:
1. The `instanceof LogicalJoin` guard does **not** protect the
`isAsofJoin()` call — if `filter.child()` is not a `LogicalJoin`, the cast
`((LogicalJoin<?, ?>) filter.child())` on the ASOF line will throw
`ClassCastException`.
2. Any filter whose child is ASOF (regardless of `instanceof` check)
triggers `whenNot`, which may not be the intended behavior for the overall rule
predicate.
Compare with the correct guard at line 166 in `joinToMultiJoin()`:
```java
if (join.isMarkJoin() || join.getJoinType().isAsofJoin()) {
return plan;
}
```
**Fix**: Move the closing parenthesis to wrap both conditions inside `&&`:
```java
.whenNot(filter -> filter.child() instanceof LogicalJoin
&& (((LogicalJoin<?, ?>) filter.child()).isMarkJoin()
|| ((LogicalJoin<?, ?>)
filter.child()).getJoinType().isAsofJoin()))
```
Note: In practice, the `subTree(LogicalJoin.class, LogicalFilter.class)`
pattern may guarantee the child is always a `LogicalJoin` or `LogicalFilter`,
so the ClassCastException risk may be mitigated. But the parenthesization is
still semantically incorrect and should be fixed.
##########
be/src/pipeline/exec/join/process_hash_table_probe_impl.h:
##########
@@ -208,6 +221,247 @@ typename HashTableType::State
ProcessHashTableProbe<JoinOpType>::_init_probe_sid
return typename HashTableType::State(_parent->_probe_columns);
}
+// ASOF JOIN optimized: two-phase probe with prefetch and compile-time
dispatch.
+// Phase 1: Batch hash chain walk — resolve all probe rows to matching build
rows.
+// Phase 2: Batch binary search — find ASOF best match for each resolved row.
+// This separation improves cache locality since phase 2 accesses
AsofIndexGroup
+// data sequentially without interleaving hash chain walks.
+template <int JoinOpType>
+template <typename HashTableType>
+uint32_t ProcessHashTableProbe<JoinOpType>::
+ _find_batch_asof_optimized( //
NOLINT(readability-function-cognitive-complexity)
+ HashTableType& hash_table_ctx, const uint8_t* null_map,
uint32_t probe_rows) {
+ auto* shared_state = _parent->_shared_state;
+ constexpr bool is_outer_join = is_asof_outer_join_op_v<JoinOpType>;
+ auto& probe_index = _parent->_probe_index;
+
+ // asof_index_groups only populated by build_asof_index when build block
has real data
+ if
(std::holds_alternative<std::monostate>(shared_state->asof_index_groups)) {
+ if constexpr (is_outer_join) {
+ uint32_t matched_cnt = 0;
+ for (; probe_index < probe_rows && matched_cnt < _batch_size;
++probe_index) {
+ _probe_indexs.get_element(matched_cnt) = probe_index;
+ _build_indexs.get_element(matched_cnt) = 0;
+ matched_cnt++;
+ }
+ return matched_cnt;
+ }
+ probe_index = probe_rows;
+ return 0;
+ }
+
+ // Get ASOF probe expression from operator and execute on probe block.
+ auto& asof_probe_expr = _parent_operator->_asof_probe_expr;
+ DORIS_CHECK(asof_probe_expr);
+
+ auto& probe_block = _parent->_probe_block;
+ int probe_col_idx = -1;
+ {
+ SCOPED_TIMER(_asof_probe_expr_timer);
+ auto st = asof_probe_expr->execute(&probe_block, &probe_col_idx);
Review Comment:
**Minor: Probe expression is re-executed on every `process()` call.**
When `_batch_size < probe_rows`, `process()` is called multiple times for
the same probe block. Each call re-executes
`asof_probe_expr->execute(&probe_block, &probe_col_idx)` here.
For `VSlotRef` (the common case — simple column reference like
`t1.timestamp`), this is essentially a no-op (just returns the column index).
However, for complex expressions (e.g., `DAYS_ADD(t1.ts, 1)`), each call to
`VExprContext::execute()` appends a **new result column** to `probe_block`,
wasting memory and compute.
Consider caching `probe_col_idx` after the first execution and skipping
re-execution on subsequent calls. For example, store the computed column index
in a member variable and only execute when it hasn't been computed for the
current probe block.
--
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]
