github-actions[bot] commented on code in PR #66371:
URL: https://github.com/apache/doris/pull/66371#discussion_r3701835732
##########
fe/fe-core/src/main/java/org/apache/doris/qe/VariableMgr.java:
##########
@@ -154,7 +154,8 @@ public class VariableMgr {
"enable_common_expr_pushdown_for_inverted_index",
"enable_phrase_query_sequential_opt",
"enable_rust_lance_reader",
- "shuffled_agg_node_ids");
+ "shuffled_agg_node_ids",
+ "plan_nereids_dump");
Review Comment:
This removed-name fence does not cover `UNSET VARIABLE`. Both
`UnsetVariableCommand.run()` and `afterForwardToMaster()` call
`getDefaultValue()` first; after the annotation disappears it returns null and
raises `ERR_UNKNOWN_SYSTEM_VARIABLE` before `setVar()` can no-op. Please make
session and forwarded/global UNSET of removed names succeed as no-ops, while
preserving errors for genuinely unknown names, and cover both paths.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/VariableMgr.java:
##########
@@ -154,7 +154,8 @@ public class VariableMgr {
"enable_common_expr_pushdown_for_inverted_index",
"enable_phrase_query_sequential_opt",
"enable_rust_lance_reader",
- "shuffled_agg_node_ids");
+ "shuffled_agg_node_ids",
+ "plan_nereids_dump");
Review Comment:
Please normalize removed-variable names with `Locale.ROOT` (or compare
locale-independently). `toLowerCase()` uses the FE's default locale, so on
Turkish/Azeri-locale FEs uppercase `PLAN_NEREIDS_DUMP` contains a dotless
lowercase I and misses this ASCII entry, whereas the old live-variable registry
was locale-independent. Add an uppercase non-English-locale case.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/VariableMgr.java:
##########
@@ -154,7 +154,8 @@ public class VariableMgr {
"enable_common_expr_pushdown_for_inverted_index",
"enable_phrase_query_sequential_opt",
"enable_rust_lance_reader",
- "shuffled_agg_node_ids");
+ "shuffled_agg_node_ids",
+ "plan_nereids_dump");
Review Comment:
Please enforce removed-name semantics before raw operations can be forwarded
to an old master. During an FE rolling upgrade, a new follower forwards `SET
GLOBAL plan_nereids_dump=true` before `VariableMgr.setVar()` runs, and it can
also forward raw `SELECT @@plan_nereids_dump` or
`SET_VAR(plan_nereids_dump=true)` queries when it cannot read or query
forwarding is forced. The old master still registers this variable, so it
applies/exposes the old behavior and SET_VAR enters replay-only planner mode. A
version-aware forwarding contract must also preserve valid assignments in mixed
SET statements; please cover command and forced-query forwarding.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -2565,8 +2563,10 @@ public static boolean isEagerAggregationOnJoin() {
@VarAttrDef.VarAttr(name = ENABLE_FOLD_NONDETERMINISTIC_FN)
public boolean enableFoldNondeterministicFn = false;
- @VarAttrDef.VarAttr(name = PLAN_NEREIDS_DUMP)
- public boolean planNereidsDump = false;
+ // Internal state, not a session variable: it is turned on only by
MinidumpUtils while replaying
Review Comment:
Please add enabled coverage for this internal-only boundary. The
authoritative diff changes no test, the existing removed-variable regression
matrix omits `plan_nereids_dump`, and the only test that loads and executes a
minidump is disabled. At minimum, verify the name is absent from
registry/JSON/forward/map output, old JSON remains readable, ordinary sessions
stay false, and `MinidumpUtils.setConnectContext()` makes the replay session
true before planner consumers run; the compatibility fixes should also receive
direct SET/SELECT/UNSET/replay coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/VariableMgr.java:
##########
@@ -154,7 +154,8 @@ public class VariableMgr {
"enable_common_expr_pushdown_for_inverted_index",
"enable_phrase_query_sequential_opt",
"enable_rust_lance_reader",
- "shuffled_agg_node_ids");
+ "shuffled_agg_node_ids",
+ "plan_nereids_dump");
Review Comment:
Historical `SET GLOBAL plan_nereids_dump` records do not use this
compatibility path during journal replay. `replayGlobalVariableV2()` finds no
registry entry and emits an ERROR for every old record, even though removal is
expected and replay continues. Please recognize removed names before the
unknown-variable error (retaining that error for genuinely unknown keys) and
add a raw legacy `GlobalVarPersistInfo` replay test.
--
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]
