github-actions[bot] commented on code in PR #66329:
URL: https://github.com/apache/doris/pull/66329#discussion_r3703734124
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowColumnsCommand.java:
##########
@@ -47,16 +47,18 @@
import com.google.common.collect.Lists;
import java.util.ArrayList;
+import java.util.HashMap;
import java.util.List;
import java.util.Locale;
+import java.util.Map;
/**
* Represents the SHOW COLUMNS command.
*/
public class ShowColumnsCommand extends ShowCommand {
private static final ShowResultSetMetaData META_DATA =
ShowResultSetMetaData.builder()
.addColumn(new Column("Field", ScalarType.createVarchar(20)))
- .addColumn(new Column("Type", ScalarType.createVarchar(20)))
+ .addColumn(new Column("Type", ScalarType.createStringType()))
Review Comment:
[P1] Keep the SHOW type column character-typed
`ScalarType.createStringType()` is not just an unbounded VARCHAR in the
MySQL protocol: `PrimitiveType.STRING.toMysqlType()` advertises the BLOB-family
field code 252, while the previous VARCHAR advertises code 254 (`STRING`). This
changes the observable result-set metadata even though the row text is
unchanged. Row serialization is already length-encoded, so please widen this
with a character type such as `createVarchar(MAX_VARCHAR_LENGTH)` (and do the
same in the verbose metadata) instead of changing the wire type.
##########
be/src/information_schema/schema_columns_scanner.cpp:
##########
@@ -210,20 +210,18 @@ std::string
SchemaColumnsScanner::_type_to_string(TColumnDesc& desc) {
return "datetime";
case TPrimitiveType::DECIMALV2: {
return fmt::format(
- "decimal({}, {})",
+ "decimal({},{})",
desc.__isset.columnPrecision ?
std::to_string(desc.columnPrecision) : "27",
desc.__isset.columnScale ? std::to_string(desc.columnScale) :
"9");
}
case TPrimitiveType::DECIMAL32:
case TPrimitiveType::DECIMAL64:
case TPrimitiveType::DECIMAL128I:
case TPrimitiveType::DECIMAL256: {
- fmt::memory_buffer debug_string_buffer;
- fmt::format_to(
- debug_string_buffer, "decimalv3({}, {})",
+ return fmt::format(
+ "decimal({},{})",
Review Comment:
[P1] Preserve Decimal metadata across mixed BE versions
Schema scans run on one eligible backend, but `TColumnDesc` has no
formatter-version fence: an old BE emits `decimalv3(p, s)` and this BE emits
`decimal(p,s)`. During a BE rolling upgrade, direct
`information_schema.columns` results and predicates therefore depend on which
BE executes the scan; if a new FE is active before every BE is upgraded, the
added `SHOW ... WHERE Type = 'decimal(10,2)'` path inherits the same split.
Please add a mixed-version compatibility strategy so metadata text is not
selected by the executing BE binary.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowColumnsCommand.java:
##########
@@ -138,9 +140,25 @@ public Expression visitUnboundSlot(UnboundSlot slot, Void
context) {
@Override
public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws Exception {
validate(ctx);
+ String ctl = tableNameInfo.getCtl();
+ DatabaseIf db =
Env.getCurrentEnv().getCatalogMgr().getCatalogOrAnalysisException(ctl)
+ .getDbOrAnalysisException(tableNameInfo.getDb());
+ TableIf table = db.getTableOrAnalysisException(tableNameInfo.getTbl());
if (whereClause != null) {
+ Map<String, String> visibleColumnTypes = new HashMap<>();
Review Comment:
[P1] Keep the displayed type in the scan's schema snapshot
This map is captured under the target table's read lock, but the lock is
released before `executePlan()` starts a separate information-schema scan and
takes a later schema snapshot. A concurrent ALTER (or drop/recreate with the
same name) can therefore return later `Null`/key/default fields while the
post-processing step substitutes the earlier type; an added or renamed column
produces a null lookup. Please produce the predicate fields and displayed type
from one version-fenced metadata source rather than joining snapshots by column
name.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowColumnsCommand.java:
##########
@@ -138,9 +140,25 @@ public Expression visitUnboundSlot(UnboundSlot slot, Void
context) {
@Override
public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws Exception {
validate(ctx);
+ String ctl = tableNameInfo.getCtl();
+ DatabaseIf db =
Env.getCurrentEnv().getCatalogMgr().getCatalogOrAnalysisException(ctl)
+ .getDbOrAnalysisException(tableNameInfo.getDb());
+ TableIf table = db.getTableOrAnalysisException(tableNameInfo.getTbl());
if (whereClause != null) {
+ Map<String, String> visibleColumnTypes = new HashMap<>();
+ table.readLock();
+ try {
+ for (Column column : table.getBaseSchema()) {
+ visibleColumnTypes.put(column.getName(),
+
column.getOriginType().hideVersionForVersionColumn(false));
+ }
+ } finally {
+ table.readUnlock();
+ }
+
Expression rewritten = whereClause.accept(new
ReplaceColumnNameVisitor(), null);
- String whereCondition = " WHERE TABLE_NAME = '" +
tableNameInfo.getTbl() + "' AND " + rewritten.toSql();
+ String whereCondition = " WHERE TABLE_SCHEMA = '" +
tableNameInfo.getDb()
Review Comment:
[P1] Match the external catalog's published schema name
`TABLE_SCHEMA` is produced from the resolved external database, but this
predicate uses the caller spelling. With
`show_full_db_name_in_info_schema_db=true`, the scanner publishes `catalog.db`
while this compares plain `db`; with `lower_case_database_names=2`, lookup can
resolve `sales` to canonical `Sales` while the predicate still compares
`sales`. The pushed database pattern can eliminate the database before rows are
produced, and the residual equality fails too. Please filter on a
presentation-independent database identity (or exactly the published name) and
test both external-catalog settings.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowColumnsCommand.java:
##########
@@ -166,17 +184,12 @@ public ShowResultSet doRun(ConnectContext ctx,
StmtExecutor executor) throws Exc
LogicalPlan plan = Utils.buildLogicalPlan(selectList, info,
whereCondition);
List<List<String>> rows = Utils.executePlan(ctx, executor, plan);
for (List<String> row : rows) {
- String rawType = row.get(1);
- row.set(1, normalizeSqlColumnType(rawType));
+ row.set(1, visibleColumnTypes.get(row.get(0)));
Review Comment:
[P1] Filter on the same Type value that is returned
The `WHERE Type` predicate is evaluated against BE `COLUMN_TYPE` before this
replacement. For a STRING column the scanner supplies `string` while this map
now supplies `text`: `WHERE Type = 'text'` returns no row, whereas `WHERE Type
= 'string'` returns a row displayed as `text`, which does not satisfy its
predicate. Named STRUCTs have the same newly introduced mismatch because the
scanner omits field names while the replacement preserves them. Please evaluate
the predicate and construct the result from one canonical type representation,
and cover non-Decimal cases.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowColumnsCommand.java:
##########
@@ -138,9 +140,25 @@ public Expression visitUnboundSlot(UnboundSlot slot, Void
context) {
@Override
public ShowResultSet doRun(ConnectContext ctx, StmtExecutor executor)
throws Exception {
validate(ctx);
+ String ctl = tableNameInfo.getCtl();
+ DatabaseIf db =
Env.getCurrentEnv().getCatalogMgr().getCatalogOrAnalysisException(ctl)
+ .getDbOrAnalysisException(tableNameInfo.getDb());
+ TableIf table = db.getTableOrAnalysisException(tableNameInfo.getTbl());
if (whereClause != null) {
+ Map<String, String> visibleColumnTypes = new HashMap<>();
+ table.readLock();
+ try {
+ for (Column column : table.getBaseSchema()) {
+ visibleColumnTypes.put(column.getName(),
+
column.getOriginType().hideVersionForVersionColumn(false));
+ }
+ } finally {
+ table.readUnlock();
+ }
+
Expression rewritten = whereClause.accept(new
ReplaceColumnNameVisitor(), null);
- String whereCondition = " WHERE TABLE_NAME = '" +
tableNameInfo.getTbl() + "' AND " + rewritten.toSql();
+ String whereCondition = " WHERE TABLE_SCHEMA = '" +
tableNameInfo.getDb()
+ + "' AND TABLE_NAME = '" + tableNameInfo.getTbl() + "' AND
" + rewritten.toSql();
Review Comment:
[P1] Encode the database as a SQL literal
External catalog database names are not constrained by internal `CREATE
DATABASE` validation; JDBC imports `TABLE_SCHEM` verbatim. A valid
backtick-addressed external schema such as `sales'2026` makes this generated
text `TABLE_SCHEMA = 'sales'2026'`, so `buildLogicalPlan()` reparses a broken
or structurally changed predicate. Please build this condition structurally or
apply the repository's SQL string-literal escaping instead of concatenating the
analyzed name.
##########
regression-test/suites/query_p0/show/test_show_columns_command.groovy:
##########
@@ -50,10 +53,31 @@ suite("test_show_columns_command", "query_p0") {
checkNereidsExecute("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE
Field = 'id'""")
qt_cmd("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE Field =
'id'""")
+ // WHERE must preserve the user-visible type, including DecimalV3 and
DATETIMEV2 precision.
+ checkNereidsExecute("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE
Field LIKE 'decimal%'""")
+ qt_cmd("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE Field LIKE
'decimal%'""")
+ checkNereidsExecute("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE
Field = 'event_time_v2'""")
+ qt_cmd("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE Field =
'event_time_v2'""")
+ checkNereidsExecute("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE
Type = 'decimal(10,2)'""")
+ qt_cmd("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE Type =
'decimal(10,2)'""")
+
// Test SHOW FULL COLUMNS with WHERE clause
checkNereidsExecute("""SHOW FULL COLUMNS FROM ${dbName}.${tableName}
WHERE Field LIKE '%name%'""")
+ // A same-name table in another database must not affect the WHERE
path.
+ sql """CREATE DATABASE IF NOT EXISTS ${otherDbName}"""
+ sql """
+ CREATE TABLE IF NOT EXISTS ${otherDbName}.${tableName} (
+ other_db_only INT
+ )
+ DISTRIBUTED BY HASH(other_db_only) BUCKETS 1
+ PROPERTIES ("replication_num" = "1");
+ """
+ qt_cmd("""SHOW COLUMNS FROM ${dbName}.${tableName} WHERE Field LIKE
'%'""")
+
} finally {
+ sql """DROP TABLE IF EXISTS ${otherDbName}.${tableName}"""
Review Comment:
[P2] Do not let cleanup replace the test failure
If any SHOW check above fails before `otherDbName` is created, this first
`finally` statement throws too: `DROP TABLE IF EXISTS db.table` resolves the
database before applying the table-level `IF EXISTS`, so a missing database is
still an error. That cleanup error obscures the original failure and prevents
the remaining main-table cleanup from running. Please create/reset the second
database before entering the failure-prone block or otherwise make cleanup
preserve the original exception.
--
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]
