github-actions[bot] commented on code in PR #68299:
URL: https://github.com/apache/doris/pull/68299#discussion_r4059071149
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowCatalogCommand.java:
##########
@@ -65,6 +84,26 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
.showCatalogs(catalogName, pattern, ctx.getCurrentCatalog() !=
null
? ctx.getCurrentCatalog().getName() : null);
+ if (whereClause == null || rows.isEmpty()) {
+ return new ShowResultSet(getMetaData(), rows);
+ }
+
+ // Apply WHERE only after the existing catalog privilege filter has
produced the rows.
+ List<List<NamedExpression>> values = new ArrayList<>(rows.size());
+ for (List<String> row : rows) {
+ values.add(ImmutableList.of(
+ new Alias(new BigIntLiteral(Long.parseLong(row.get(0))),
"CatalogId"),
+ new Alias(new StringLiteral(row.get(1)), "CatalogName"),
+ new Alias(new StringLiteral(row.get(2)), "Type"),
+ new Alias(new StringLiteral(row.get(3)), "IsCurrent"),
+ new Alias(new StringLiteral(row.get(4)), "CreateTime"),
+ new Alias(new StringLiteral(row.get(5)), "LastUpdateTime"),
+ new Alias(new StringLiteral(row.get(6)), "Comment"),
+ new Alias(new StringLiteral(row.get(7)), "ErrorMsg")));
+ }
+ LogicalPlan plan = new LogicalFilter<>(ImmutableSet.of(whereClause),
new UnboundInlineTable(values));
+ plan = new LogicalSort<>(ImmutableList.of(new OrderKey(new
UnboundSlot("CatalogName"), true, true)), plan);
Review Comment:
[P2] Sorting again here does not preserve the order that
`CatalogMgr.showCatalogs()` already defines for every legal catalog name. The
manager uses Java `String.compareTo` (UTF-16), whereas this `STRING` sort
reaches BE bytewise UTF-8 comparison. Unicode names are enabled by default and
supplementary letters are accepted, so names with a shared prefix followed by
U+10400 and U+F900 reverse relative order merely by adding a WHERE clause.
Please retain and sort by the original row ordinal (then project it away), or
filter without re-sorting.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowCatalogCommand.java:
##########
@@ -65,6 +84,26 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
.showCatalogs(catalogName, pattern, ctx.getCurrentCatalog() !=
null
? ctx.getCurrentCatalog().getName() : null);
+ if (whereClause == null || rows.isEmpty()) {
Review Comment:
[P2] This early return makes predicate analysis depend on authorization
results. A configured system-scope authorization plugin can legitimately hide
every catalog; in that session `SHOW CATALOGS WHERE no_such_column = 1` reaches
this branch and succeeds with an empty result, while the same statement is
rejected as an unknown column as soon as one catalog is visible. Please
bind/type-check the predicate against the fixed SHOW schema even for an empty
input, then skip only runtime evaluation.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowCatalogCommand.java:
##########
@@ -65,6 +84,26 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
.showCatalogs(catalogName, pattern, ctx.getCurrentCatalog() !=
null
? ctx.getCurrentCatalog().getName() : null);
+ if (whereClause == null || rows.isEmpty()) {
+ return new ShowResultSet(getMetaData(), rows);
+ }
+
+ // Apply WHERE only after the existing catalog privilege filter has
produced the rows.
+ List<List<NamedExpression>> values = new ArrayList<>(rows.size());
+ for (List<String> row : rows) {
+ values.add(ImmutableList.of(
+ new Alias(new BigIntLiteral(Long.parseLong(row.get(0))),
"CatalogId"),
+ new Alias(new StringLiteral(row.get(1)), "CatalogName"),
+ new Alias(new StringLiteral(row.get(2)), "Type"),
+ new Alias(new StringLiteral(row.get(3)), "IsCurrent"),
+ new Alias(new StringLiteral(row.get(4)), "CreateTime"),
+ new Alias(new StringLiteral(row.get(5)), "LastUpdateTime"),
+ new Alias(new StringLiteral(row.get(6)), "Comment"),
+ new Alias(new StringLiteral(row.get(7)), "ErrorMsg")));
+ }
+ LogicalPlan plan = new LogicalFilter<>(ImmutableSet.of(whereClause),
new UnboundInlineTable(values));
+ plan = new LogicalSort<>(ImmutableList.of(new OrderKey(new
UnboundSlot("CatalogName"), true, true)), plan);
+ rows = Utils.executePlan(ctx, executor, new UnboundResultSink<>(plan));
Review Comment:
[P1] `Utils.executePlan` is being called with the user's outer executor, but
that helper is not lifecycle-neutral. It replaces `parsedStmt`, and
`executeInternalQueryCommon()` assigns a new query id, sets the shared
`QueryState` to `internal`, and emits its own audit without restoring that
state. `QueryState.reset()` does not clear `internal`, so after a non-empty
`SHOW CATALOGS WHERE`, the outer audit and later statements on the connection
are marked internal: normal metrics/audits are corrupted and later SQL
block-rule/scan-limit checks are skipped. Please isolate the nested execution
context or snapshot/restore all outer state and avoid the duplicate inner audit.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowCatalogCommand.java:
##########
@@ -65,6 +84,26 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
.showCatalogs(catalogName, pattern, ctx.getCurrentCatalog() !=
null
? ctx.getCurrentCatalog().getName() : null);
+ if (whereClause == null || rows.isEmpty()) {
+ return new ShowResultSet(getMetaData(), rows);
+ }
+
+ // Apply WHERE only after the existing catalog privilege filter has
produced the rows.
+ List<List<NamedExpression>> values = new ArrayList<>(rows.size());
+ for (List<String> row : rows) {
+ values.add(ImmutableList.of(
+ new Alias(new BigIntLiteral(Long.parseLong(row.get(0))),
"CatalogId"),
+ new Alias(new StringLiteral(row.get(1)), "CatalogName"),
+ new Alias(new StringLiteral(row.get(2)), "Type"),
+ new Alias(new StringLiteral(row.get(3)), "IsCurrent"),
+ new Alias(new StringLiteral(row.get(4)), "CreateTime"),
Review Comment:
[P1] Missing `CreateTime`/`LastUpdateTime` values are represented in the
SHOW rows by `FeConstants.null_string` and sent to clients as SQL NULL (the
default internal catalog has both), but these lines turn them into non-nullable
string literals before applying WHERE. As a result, predicates such as
`CreateTime IS NULL` or `LastUpdateTime <=> NULL` fold false; old-image null
comments have the same mismatch. Please construct typed `NullLiteral`s for
null/sentinel cells and add coverage for the default row and restored metadata.
--
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]
