github-actions[bot] commented on code in PR #60901:
URL: https://github.com/apache/doris/pull/60901#discussion_r2887462149
##########
fe/fe-core/src/main/java/org/apache/doris/alter/RollupJobV2.java:
##########
@@ -889,7 +889,6 @@ private void setColumnsDefineExpr(List<MVColumnItem>
mvColumnItemList) {
for (Column column : rollupSchema) {
if (column.getName().equals(mvColumnItem.getName())) {
Expr defineExpr = mvColumnItem.getDefineExpr();
- defineExpr.disableTableName();
column.setDefineExpr(defineExpr);
Review Comment:
Potential behavioral change: The old code called
`defineExpr.disableTableName()` before storing the expr on the column. This
ensured that `Column.getDisplayName()` (which calls
`defineExpr.accept(ExprToSqlVisitor.INSTANCE, null)`) would produce output
without table qualifications.
Now that `disableTableName()` is removed and
`ExprToSqlVisitor.visitSlotRef()` unconditionally includes `tableNameInfo` when
it is non-null, `getDisplayName()` may produce `tableName.columnName` instead
of just `columnName`.
Consider changing `Column.getDisplayName()` to use:
```java
defineExpr.accept(ExprToExternalSqlVisitor.INSTANCE,
ToSqlParams.WITHOUT_TABLE)
```
to preserve the old table-name-stripping behavior.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/ExprToColumnLabelVisitor.java:
##########
@@ -0,0 +1,60 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.analysis;
+
+/**
+ * Visitor that produces the column-label string for any {@link Expr}.
+ *
+ * <p>For all expression types except {@link SlotRef}, the result is identical
+ * to {@link ExprToSqlVisitor} (i.e. the standard SQL string). For
+ * {@link SlotRef} the result is the raw column name without table
qualification
+ * or backtick quoting, matching the legacy {@code SlotRef.toColumnLabel()}
+ * behaviour.
+ *
+ * <p>Usage:
+ * <pre>
+ * String label = expr.accept(ExprToColumnLabelVisitor.INSTANCE, null);
+ * </pre>
+ */
+public class ExprToColumnLabelVisitor extends ExprVisitor<String, Void> {
+
+ /** Singleton — the visitor is stateless. */
+ public static final ExprToColumnLabelVisitor INSTANCE = new
ExprToColumnLabelVisitor();
+
+ private ExprToColumnLabelVisitor() {
+ }
+
+ /**
+ * Default: delegate to {@link ExprToSqlVisitor} for every expression type
+ * that does not need special column-label handling.
+ */
+ @Override
+ public String visit(Expr expr, Void context) {
+ return ExprToSqlVisitor.INSTANCE.visit(expr, null);
Review Comment:
Bug: `ExprToSqlVisitor.INSTANCE.visit(expr, null)` calls the catch-all
`visit()` method on `ExprToSqlVisitor`, which throws
`UnsupportedOperationException`. This bypasses the visitor dispatch entirely.
Should be:
```java
return expr.accept(ExprToSqlVisitor.INSTANCE, null);
```
This properly dispatches through each Expr subclass's `accept()` method to
the correct `visitXxx` handler. Currently this bug is latent because the only
call site is `SlotRef.getExprName()` which dispatches to the overridden
`visitSlotRef` and never hits this fallback. However, the class Javadoc
explicitly claims it works for "any Expr", and future callers following that
guidance will crash.
##########
fe/fe-core/src/main/java/org/apache/doris/load/routineload/KafkaRoutineLoadJob.java:
##########
@@ -953,20 +955,26 @@ public Long totalLag() {
Review Comment:
Note: This is a semantic change from the old code. Previously
`getDeleteCondition().toSql()` was used (standard SQL path, preserving table
names unless the instance `disableTableName` flag was set). Now
`ExprToExternalSqlVisitor.INSTANCE` with `ToSqlParams.WITHOUT_TABLE` is used,
which always strips table names.
The same change was made for `precedingFilter` below. This is likely correct
since the result is parsed by `NereidsLoadUtils.parseExpressionSeq()` which
expects plain expressions, and it makes `deleteCondition`/`precedingFilter`
consistent with `whereExpr` which already used `toSqlWithoutTbl()`. Just
confirming this is intentional.
--
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]
