github-actions[bot] commented on code in PR #61574:
URL: https://github.com/apache/doris/pull/61574#discussion_r2964909434
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/ExprToSqlVisitor.java:
##########
@@ -175,30 +174,15 @@ public String visitSlotRef(SlotRef expr, ToSqlParams
context) {
&& ConnectContext.get().getState().isNereids()
&& !ConnectContext.get().getState().isQuery()
&& ConnectContext.get().getSessionVariable() != null
- && expr.desc != null) {
- return expr.getLabel() + "[#" + expr.desc.getId().asInt() +
"]";
+ && expr.getSlotId() != null) {
+ return expr.getLabel() + "[#" + expr.getSlotId().asInt() + "]";
} else {
return expr.getLabel();
}
- } else if (expr.desc == null) {
+ } else if (expr.getSlotId() == null) {
return "`" + expr.getCol() + "`";
Review Comment:
**[BUG]** Same issue as above: `expr.getSlotId() == null` will throw
`NullPointerException` when `desc` is null, because `SlotRef.getSlotId()`
internally calls `Preconditions.checkNotNull(desc)`. The old code used
`expr.desc == null` which was safe.
Should be: `expr.getDesc() == null`
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/ExprToSqlVisitor.java:
##########
@@ -175,30 +174,15 @@ public String visitSlotRef(SlotRef expr, ToSqlParams
context) {
&& ConnectContext.get().getState().isNereids()
&& !ConnectContext.get().getState().isQuery()
&& ConnectContext.get().getSessionVariable() != null
- && expr.desc != null) {
- return expr.getLabel() + "[#" + expr.desc.getId().asInt() +
"]";
+ && expr.getSlotId() != null) {
+ return expr.getLabel() + "[#" + expr.getSlotId().asInt() + "]";
Review Comment:
**[BUG]** `expr.getSlotId()` calls `Preconditions.checkNotNull(desc)`
internally (see `SlotRef.java:130`). If `desc` is null, this will throw
`NullPointerException` instead of evaluating to `null`. The old code `expr.desc
!= null` was a safe null check.
This line and the one at line 182 (`expr.getSlotId() == null`) should use
`expr.getDesc() != null` / `expr.getDesc() == null` instead, to preserve the
original null-safety semantics.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/expression/rules/FoldConstantRuleOnBE.java:
##########
@@ -146,7 +144,7 @@ public static boolean
isEnableFoldByBe(ExpressionMatchingContext<Expression> ctx
/** foldByBE */
public static Expression foldByBE(ExpressionMatchingContext<Expression>
context) {
- IdGenerator<ExprId> idGenerator = ExprId.createGenerator();
+ Integer idGenerator = 0;
Review Comment:
**[CRITICAL BUG]** `Integer` is immutable in Java and passed by value.
Replacing `IdGenerator<ExprId>` (a mutable object with shared state) with
`Integer` breaks the ID generation logic.
When `collectConst` is called recursively for sibling children in the `else`
branch (line 223), each sibling receives the **same `id` value** because the
`id++` on line 205 only modifies the local copy — the increment is invisible to
the caller and subsequent siblings.
**Example:** For `f(g(1), h(2))` where both `g(1)` and `h(2)` are constant:
1. `collectConst(f(...), ..., id=0)` → not fully constant, enters `else`
2. `collectConst(g(1), ..., id=0)` → constant, `id++` → local `id=1`, puts
`"1" → g(1)`
3. `collectConst(h(2), ..., id=0)` → same `id=0` from parent! → `id++` →
local `id=1`, puts `"1" → h(2)` **overwriting g(1)**
**Result:** `g(1)` is silently lost from the map. The `replace()` phase may
substitute `g(1)`'s position with `h(2)`'s folded result, producing **wrong
query results**.
**Fix:** Use `AtomicInteger` (already imported in this file!) instead of
`Integer`:
```java
AtomicInteger idGenerator = new AtomicInteger(0);
// ...
String id = String.valueOf(idGenerator.incrementAndGet());
```
--
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]
