github-actions[bot] commented on code in PR #60901: URL: https://github.com/apache/doris/pull/60901#discussion_r2893790378
########## fe/fe-core/src/main/java/org/apache/doris/analysis/ExprToColumnLabelVisitor.java: ########## @@ -0,0 +1,51 @@ +// 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 ExprToSqlVisitor { Review Comment: Test gap: `ExprToColumnLabelVisitor` has zero test coverage in `ExprToSqlTest.java`. Please add at least one test verifying that a `SlotRef` returns the raw column name without table qualification or backtick quoting, and one test verifying that a non-SlotRef expression (e.g., `IntLiteral`) still uses the parent `ExprToSqlVisitor` logic. Note also that the Javadoc usage example at line 31 suggests passing `null` as context: `expr.accept(ExprToColumnLabelVisitor.INSTANCE, null)`. This is only safe for `SlotRef` since `visitSlotRef` doesn't use context. For any other `Expr` type, the inherited `ExprToSqlVisitor` methods will NPE when accessing `context.disableTableName`. Consider documenting this limitation or using `ToSqlParams.WITH_TABLE` as the default context in the example. ########## fe/fe-core/src/main/java/org/apache/doris/analysis/ExprToExternalSqlVisitor.java: ########## @@ -0,0 +1,84 @@ +// 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; + +import org.apache.doris.catalog.JdbcTable; +import org.apache.doris.catalog.OdbcTable; +import org.apache.doris.catalog.TableIf; + +/** + * Visitor that produces the external SQL string for any {@link Expr}. + * Extends {@link ExprToSqlVisitor} and overrides only the methods that differ + * for external SQL generation. + * + * <p>Usage examples: + * <pre> + * // equivalent to expr.toSqlWithoutTbl() + * expr.accept(ExprToExternalSqlVisitor.INSTANCE, ToSqlParams.WITHOUT_TABLE) + * + * // equivalent to expr.toExternalSql(tableType, table) + * expr.accept(ExprToExternalSqlVisitor.INSTANCE, new ToSqlParams(false, true, tableType, table)) + * </pre> + */ +public class ExprToExternalSqlVisitor extends ExprToSqlVisitor { + + public static final ExprToExternalSqlVisitor INSTANCE = new ExprToExternalSqlVisitor(); + + private ExprToExternalSqlVisitor() { + // singleton + } + + @Override + public String visit(Expr expr, ToSqlParams context) { + throw new UnsupportedOperationException("ExprToExternalSqlVisitor does not support Expr type: " + + expr.getClass().getSimpleName()); + } + + @Override + public String visitSlotRef(SlotRef expr, ToSqlParams context) { + if (expr.getCol() != null) { + if (context.tableType.equals(TableIf.TableType.JDBC_EXTERNAL_TABLE) Review Comment: Bug: `context.tableType.equals(...)` will NPE when `context.tableType` is null. This can happen if someone follows the Javadoc example at line 32 (`ToSqlParams.WITHOUT_TABLE`, which has `tableType=null`) or passes a `ToSqlParams` with null `tableType` (as `ExprToSqlTest.java` does at lines 596/609). While no current production call site triggers this (they all pass non-null `tableType`), this is a latent trap. Suggested fix — use null-safe enum comparison: ```java if (TableIf.TableType.JDBC_EXTERNAL_TABLE.equals(context.tableType) || TableIf.TableType.JDBC.equals(context.tableType) || TableIf.TableType.ODBC.equals(context.tableType)) { ``` -- 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]
