Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: SHOW CATALOGS passed WHERE expressions to the LIKE pattern matcher, so valid predicates failed as invalid patterns. Keep LIKE patterns and WHERE expressions separate, then evaluate WHERE against the privilege-filtered SHOW rows. ### Release note Fix SHOW CATALOGS WHERE filtering. ### Check List (For Author) - Test: Unit Test; regression test added - Behavior changed: Yes. SHOW CATALOGS WHERE now filters the displayed rows. - Does this need documentation: No
Gabriel39
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
September 21, 2026 02:54
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 27393 ms |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes for four correctness and lifecycle regressions in the new SHOW CATALOGS WHERE execution path.
Checkpoint conclusions:
- Goal and scope: LIKE and WHERE parsing are now separated correctly, and the constructor/call-site scope is focused, but the new runtime reconstruction path is not behavior-preserving.
- Lifecycle, observability, and policy: the nested query mutates the user's executor and shared query state, misclassifies audits/metrics, and leaves later statements exempt from SQL block/scan-limit checks.
- Authorization and condition handling: privilege filtering still precedes WHERE, but an empty authorized result skips predicate analysis entirely.
- Compatibility and result semantics: SHOW NULL sentinels are reconstructed as non-null strings; old-image null comments have the same mismatch. The new BE sort can also change the established order of legal Unicode catalog names.
- Concurrency and snapshots: catalog rows are still collected under the existing catalog-manager read lock; no new lock-order or shared-mutation issue was found beyond the executor/context lifecycle issue above.
- Configuration, persistence, protocol, transactions, and data writes: no new configuration, write, transaction, storage-format, or FE/BE field change is introduced. Old persisted metadata was checked because it feeds the new literal conversion.
- Parallel paths and performance: singular SHOW and LIKE remain unchanged. The backend dependency and linear inline-plan growth match established SHOW-WHERE paths and were not raised separately.
- Tests: the added tests cover the reported LIKE-versus-WHERE happy path but not connection-state leakage, zero-visible-catalog analysis, SQL NULL predicates, or Unicode ordering. Per the review contract, this was a static review only; I did not run builds or tests, so author/CI validation was not independently reproduced.
- User focus: no additional review focus was supplied.
Review status: complete after three convergence rounds; all candidates were adjudicated and no unresolved point remains.
Contributor
TPC-DS: Total hot run time: 152218 ms |
Contributor
ClickBench: Total hot run time: 24.2 s |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 28148 ms |
Contributor
TPC-DS: Total hot run time: 153478 ms |
Contributor
ClickBench: Total hot run time: 23.93 s |
Contributor
There was a problem hiding this comment.
Request changes: the new SHOW CATALOGS WHERE execution path has seven distinct issues (five P1 correctness failures and two P2 concurrency/performance defects).
Checkpoint conclusions:
- Goal and proof: LIKE/WHERE parsing and the direct uncached equality cases are improved, but general WHERE semantics are not achieved: valid scalar/NULL predicates, session-dependent expressions, server-prepared parameters, repeated cached execution, and the supported dropped-current-catalog state fail or return wrong rows.
- Scope and focus: the parser change is focused, but the command adds a bespoke cloned-session and per-row full-planner pipeline instead of reusing the query-backed strategy of parallel SHOW commands. That larger mechanism is the source of the findings.
- Concurrency: CatalogMgr still snapshots privilege-filtered rows under its existing read lock, with no new lock-order problem. However, aliases are constructed before a per-row StatementContext exists, exposing the shared unsynchronized fallback ExprId generator and allowing silent wrong-field substitution under concurrent requests.
- Lifecycle: AutoCloseConnectContext fixes the previously reported outer-state mutation and planner resources are released. The clone nevertheless omits SQL-observable session state, cannot reproduce a dropped selected catalog, loses prepared bindings, and lets synthetic results escape through the global SQL cache.
- Configuration: no setting is added. Default
enable_sql_cache=truedirectly triggers the partial-result replay; the default FE constant-fold path provides no fallback for supported scalars that survive folding. - Compatibility: singular SHOW, LIKE, and the no-WHERE fast path remain intact. No persisted metadata, storage format, thrift field, function symbol, or rolling FE/BE compatibility surface changes. Server-prepared SHOW is an existing supported protocol path and regresses here.
- Parallel paths and stage placement: query-backed SHOW-WHERE implementations can execute the accepted scalar language; this one requires total FE folding and plans separately for every catalog row.
- Conditions and errors: privilege-before-filter ordering, typed zero-row binding, SQL-NULL reconstruction, direct row order, and exception restoration are sound at this head. Surviving filters throw an internal invariant, stale namespaces NPE, and missing prepared bindings fail analysis.
- Tests and claimed results: the unit tests cover parser separation, direct equality, empty-input name analysis, outer restoration, Unicode order, and NULL conversion. The regression is a one-shot smoke call without row assertions. Missing coverage includes non-FE scalars/UNKNOWN, user/session state, repeated COM_QUERY cache replay, prepared placeholders, dropped-current-catalog WHERE, concurrent ExprId uniqueness, and high catalog counts.
- Observability: no new distributed lifecycle requires metrics, but existing planner/cache telemetry would not expose the silent wrong rows from cache replay or ExprId collision.
- Transactions, persistence, and data writes: not applicable; these paths do not modify EditLog/replay, transactions, metadata, or user data.
- FE/BE variables, protocol, and memory safety: no new transmitted variable or BE memory/nullability code exists. Temporary planner resources are released; the material resource issue is the uncapped repeated planning cost.
- Performance: one SHOW performs a full Nereids analysis/optimization/translation cycle for every visible catalog, with no catalog-count bound.
- Security and other correctness: authorization still precedes filtering, but the cached synthetic row records no catalog-list or SHOW-grant dependency, so metadata/RBAC changes can replay stale visibility. Restricted-user metadata visibility is within the repository threat model.
- Existing review threads: the four earlier lifecycle, empty-input, Unicode-order, and NULL-sentinel findings are statically fixed at the reviewed head and are not duplicated below.
- User focus: no additional user-provided focus was supplied.
- Validation and status: this was static review only as required; I did not run builds or tests, so author/CI claims were not independently reproduced. The review is capped/incomplete after the third allowed round because that final round found the prepared-placeholder defect; all candidates found so far were independently adjudicated and the seven accepted issues are included inline.
| NereidsPlanner planner = new NereidsPlanner(statementContext); | ||
| planner.plan(adapter, filterContext.getSessionVariable().toThrift()); | ||
| Optional<ResultSet> resultSet = planner.handleQueryInFe(adapter); | ||
| if (!resultSet.isPresent()) { |
Contributor
There was a problem hiding this comment.
[P1] Do not require every valid WHERE predicate to fold entirely in FE. For example, SHOW CATALOGS WHERE hex(CatalogName) = '696E7465726E616C' is accepted, but hex has no FE executable registration, so the reduced tree still contains PhysicalFilter. That node is not ComputeResultSet; handleQueryInFe returns empty and this path throws the internal invariant instead of evaluating the valid predicate. Please execute the full accepted expression language (or deliberately validate a narrower language with a user-facing analysis error) and cover a non-FE-foldable scalar.
|
|
||
| private ConnectContext buildFilterContext(ConnectContext outerContext) { | ||
| ConnectContext filterContext = new ConnectContext(); | ||
| filterContext.setSessionVariable(VariableMgr.cloneSessionVariable(outerContext.getSessionVariable())); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the caller's SQL-observable session state in the isolated evaluation context. This fresh context does not copy user variables, connection id, last query id, or the original statement clock: after SET @wanted='internal', SHOW CATALOGS WHERE CatalogName=@wanted fails as an unsupported variable, and connection_id()/last_query_id() fold against default clone values. This is separate from the fixed outer-state mutation—the wrapper restores correctly, but the predicate reads a different session. Please use a complete purpose-built snapshot (while isolating mutable execution state) and add session-dependent predicate coverage.
| LogicalPlanAdapter adapter = new LogicalPlanAdapter(plan, statementContext); | ||
| NereidsPlanner planner = new NereidsPlanner(statementContext); | ||
| planner.plan(adapter, filterContext.getSessionVariable().toThrift()); | ||
| Optional<ResultSet> resultSet = planner.handleQueryInFe(adapter); |
Contributor
There was a problem hiding this comment.
[P1] Keep these synthetic per-row plans out of the global SQL cache. With default enable_sql_cache=true, every iteration uses this same full SHOW text, and handleQueryInFe caches the first one-row (or empty) result. For two matching catalogs, the first execution assembles both rows, but the next identical canonical command can be replaced before parsing by that first partial LogicalSqlCache; it also has no catalog-list or SHOW-grant dependency to invalidate after metadata/privilege changes. Please disable cache admission for this internal evaluation and add a connection-level test that executes the same two-match SHOW twice.
| filterContext.setSessionVariable(VariableMgr.cloneSessionVariable(outerContext.getSessionVariable())); | ||
| filterContext.setEnv(Env.getCurrentEnv()); | ||
| filterContext.changeDefaultCatalog(outerContext.getDefaultCatalog()); | ||
| filterContext.setDatabase(outerContext.getDatabase()); |
Contributor
There was a problem hiding this comment.
[P1] Preserve the existing dropped-current-catalog behavior when building this context. query_p0/show/test_show_catalogs.groovy explicitly switches to a catalog, drops that current catalog, and requires plain SHOW CATALOGS to remain valid. In that state line 183 copies the stale name, then setDatabase unconditionally calls getCurrentCatalog().getDb(db), so adding any WHERE clause now NPEs before filtering. Please copy the nullable namespace without resolving it against a missing catalog and extend that regression with a WHERE form.
| // Multi-row VALUES plans require BE execution. Filtering one row at a time keeps this path in FE | ||
| // and preserves the CatalogMgr order instead of applying a different SQL string collation. | ||
| List<List<String>> filteredRows = new ArrayList<>(rows.size()); | ||
| for (List<String> row : rows) { |
Contributor
There was a problem hiding this comment.
[P2] Avoid running a complete Nereids planning/translation cycle once per visible catalog. Each iteration creates a fresh planner and calls plan, which reaches analysis, rewrites, memo optimization, post-processing, fragment splitting, and physical translation; CatalogMgr.showCatalogs places no bound on the number of external catalogs. This makes one metadata filter consume planner CPU and allocations proportional to catalog count. Please bind/plan once for the row set (carrying an ordinal if needed to retain manager order), or compile the predicate once for repeated row evaluation.
| // and preserves the CatalogMgr order instead of applying a different SQL string collation. | ||
| List<List<String>> filteredRows = new ArrayList<>(rows.size()); | ||
| for (List<String> row : rows) { | ||
| filteredRows.addAll(executeFilterPlan(filterContext, filterPlan(toExpressions(row), false))); |
Contributor
There was a problem hiding this comment.
[P2] Install the per-row StatementContext before constructing these aliases. At this call site Java evaluates toExpressions(row) first, while the installed fresh ConnectContext still has no statement context, so every Alias draws from StatementScopeIdGenerator's shared test-only fallback. That generator uses an unsynchronized nextId++; concurrent SHOWs can therefore give two columns in one row the same ExprId. Because SlotReference equality is ExprId-only and generateReplaceMap uses putIfAbsent, a predicate on Type can then be substituted with the earlier CatalogName literal and silently return the wrong row set. Please allocate these IDs from the installed per-row context and add concurrent uniqueness coverage.
| } | ||
|
|
||
| private List<List<String>> executeFilterPlan(ConnectContext filterContext, LogicalPlan plan) throws Exception { | ||
| StatementContext statementContext = new StatementContext( |
Contributor
There was a problem hiding this comment.
[P1] Preserve bound server-prepared parameters in this inner statement context. COM_STMT_EXECUTE has already placed the literal for SHOW CATALOGS WHERE CatalogName = ? in the caller's StatementContext, but this replacement context starts with an empty idToPlaceholderRealExpr. Since it is no longer in prepare stage, ExpressionAnalyzer.visitPlaceholder receives no real expression and the parameterized SHOW fails during analysis instead of filtering. Please carry the execution's placeholder bindings into this isolated context (or bind the retained predicate before replacing it) and add a server-prepared regression with a bound catalog name.
Contributor
FE UT Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SHOW CATALOGS passed WHERE expressions to the LIKE pattern matcher, so valid predicates failed as invalid patterns. Keep LIKE patterns and WHERE expressions separate, then evaluate WHERE against the privilege-filtered SHOW rows.
Release note
Fix SHOW CATALOGS WHERE filtering.
Check List (For Author)