Repository navigation
feat(sql): add native query support for system tables - #20183
FrankChen021 wants to merge 12 commits into
Conversation
|
@clintropolis @gianm please help review the changes. I hope this can be merged as soon as possible so that we can move on migrating other system tables into native execution path, and push forward for #18087 and #19855 |
FrankChen021
left a comment
There was a problem hiding this comment.
Review complete: no high-confidence correctness, security, or reliability issues found in the current changes.
Reviewed 100 of 100 changed files.
Validation: focused git diff --no-ext-diff --check passed. Builds and tests were not run.
The dedicated reviewer timed out; this review was completed by the main agent from the prepared worktree.
This is an automated review by Codex GPT-5.6-Luna(max)
abhishekrb19
left a comment
There was a problem hiding this comment.
Thanks for these changes @FrankChen021! The sys.* tables have historically been slow and less performant, especially sys.segments and sys.tasks.
I wonder if we can benchmark sys.tasks with thousands of tasks to see how these changes perform at scale with the native support + filter pushdown.
Also, regarding the scope of these changes, I feel it would be helpful to break this into a few more patches for ease of review. Perhaps something like:
- Separate PRs for
sys.tasksandsys.server_properties, isolating the appropriate wirings, filter pushdown mechanisms and tests to make it work for each table. - Handle
DECOUPLEDplanner support in a separate change, sinceCOUPLEDis the default planner strategy today (andDECOUPLEDis currently undocumented).
| new QueryableModule(), | ||
| new QueryRunnerFactoryModule(), | ||
| new SegmentWranglerModule(), | ||
| new JoinableFactoryModule(), |
There was a problem hiding this comment.
Are these not needed? Wondering if this is general cleanup, outside the scope of this PR
There was a problem hiding this comment.
Not out of the scope. it's a tiny refactoring of how native query related module are installed. otherwise, for overlord, coordinator, middle manager, we will repeate these code. Now we only need the NativeQueryEngineModule installed, all dependencies are enclosed in this new module.
| `X-Druid-Native-Query-Route: local`. Local execution uses the authenticated request identity and applies the table's | ||
| authorization rules. The Broker uses the same header for remote node fan-out requests. If the Broker itself is | ||
| one of the selected nodes, it executes that node Scan in-process without an HTTP request. The header | ||
| controls routing and doesn't grant additional permissions. |
There was a problem hiding this comment.
Hmm, is this header required only for the sys.server_properties table? I wonder if there's a better way to achieve this without passing in additional headers.
There was a problem hiding this comment.
it's needed as the broker will send native query to router or itself to retrieve data. for router, we need to tell whether it acts as a proxy or execute the native query.
another way is by using query context, but I don't think the query context is better way because this value here is just an internal flag, not user-facing context parameter.
Thanks for reviewing. I deliberately chose the sys.tasks and sys.server_properties included in this PR to demonstrate how the native query for system tables are supported. These two system tables have different fan-out paths and push down policies:
If we move any of these system table out of this PR, we can't have a full picture of the change, and understand how the small framework for the system tables work. As for DECOUPLED mode, only a few files are involved, I think it's better to included in this PR. To address your concern, I can split the changes for sys.tasks out of this one. But We still need to have a full picture of the core change. |
Very cool. I will try to have a look soon. |
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
Reviewed 90 of 90 changed files.
This is an automated review by Codex GPT-5.6-Luna(max)
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
Rechecked the updated head against the prior reviewed SHA, beginning with the incremental patch. The prior P2 concerning nested or composite system-table datasources is resolved: local execution now rejects a non-system-table root with BadQueryContextException, and the updated tests cover both broker and node-local handlers. Composite queries without the local route continue through the recursive system-table client path.
The full current diff and surrounding code were reviewed across all 90 changed files, including SQL planning and authorization, native datasource conversion, node discovery/fanout/recovery, request routing, lifecycle and cancellation, service-module wiring, and regression tests. No additional high-confidence correctness, security, compatibility, data-loss, concurrency, lifecycle, integration, or missing-test finding was identified.
Validation performed: git diff --check 8c05f05adbe8f7ccf647c135caa948cc083fae8b -- passed. No builds, broad tests, dependency installs, formatters, prepare, fetch, or cleanup were run, per the review scope.
This is an automated review by Codex GPT-5.6-Luna(max)
…asks-mvp # Conflicts: # server/src/test/java/org/apache/druid/client/DirectDruidClientTest.java
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
Rechecked the current head beginning with the incremental patch. The incremental patch is the merge of current master into the previously reviewed PR head; the PR logic is unchanged since the prior review. The prior P2 about composite or nested system-table datasources remains resolved: local routing rejects a non-system-table root, while non-local Broker execution recursively resolves system-table leaves.
Reviewed 90 of 90 changed files. The full current diff and relevant surrounding code were reviewed across SQL planner selection and authorization, native datasource conversion, node discovery and leader routing, request forwarding, local and remote execution, failure recovery, cancellation and lifecycle cleanup, service-module wiring, and regression tests. No additional high-confidence correctness, security, compatibility, data-loss, concurrency, lifecycle, integration, or missing-test issue was found.
Validation: git diff --check cf84e5f1b09ce3b05c2f5c015e5259bf66b7a7e6 -- passed. No builds, broad tests, dependency installs, formatters, prepare, fetch, or cleanup were run.
This is an automated review by Codex GPT-5.6-Luna(max)
…asks-mvp # Conflicts: # processing/src/main/java/org/apache/druid/query/rowsandcols/LazilyDecoratedRowsAndColumns.java # services/src/main/java/org/apache/druid/cli/CliPeon.java
FrankChen021
left a comment
There was a problem hiding this comment.
🟢 Approval recommended
Rechecked the incremental patch first. It is the merge of current master into the previously reviewed PR head, so it contains upstream churn rather than additional PR logic; I then revalidated the full current PR diff and the prior composite/nested system-table routing finding at the current head.
Reviewed 90 of 90 changed files. Coverage included SQL planner native/bindable selection and authorization, native datasource conversion, system-table descriptors and providers, node discovery and leader routing, Router/Broker/local forwarding, local and remote execution, failure recovery, cancellation and lifecycle cleanup, service-module wiring, and regression and embedded tests.
Validation: git diff --check 631f05de5d1d05edfbbab3adc8dbbb99745678ee 0e7033010c84ac45dc909e36d843046fd18bb919 -- passed. No broad builds or test suites were run per review scope.
No actionable PR-caused correctness, security, compatibility, data-loss, concurrency, lifecycle, integration, or high-confidence missing-test issue found. The prior composite/nested system-table routing finding remains resolved by the current root-datasource checks and recursive non-local resolution.
This is an automated review by Codex GPT-5.6-Luna(max)
Merge tests that exercise the same code path into parameterized tests and drop tests fully covered by stronger equivalents.
Derive native system table names from the logical plan instead of building the native query, and collapse identical branches in RecoveringNodeIterator.
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
Fix virtual-column handling in provider filter pushdown before merging: filters on virtual columns named server or service_name can currently discard matching rows by comparing against physical node metadata. The incremental authorization-plan traversal and recovery simplification introduce no additional confirmed issue.
Reviewed 90 of 90 changed files, starting with the 6-file incremental diff and then reviewing the full current diff and relevant surrounding code. Rechecked the prior composite/nested system-table routing finding; it remains resolved by the local root checks and recursive Broker resolution. Coverage included SQL planning and authorization, native datasource conversion, node discovery and fanout, request routing, recovery, cancellation and cleanup, module wiring, and regression tests.
Validation: git diff --check 631f05de5d1d05edfbbab3adc8dbbb99745678ee HEAD -- passed. This was a static review; builds and test suites were not run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 0 |
| P2 | 1 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
A virtual column shadows the physical column of the same name, so a filter on it must be evaluated by the native scan instead of being compared against the provider's physical column.
FrankChen021
left a comment
There was a problem hiding this comment.
🟡 Changes recommended
SystemTableQueryClient can throw an NPE when constructing a filtered WindowOperatorQuery whose scan leaf has no virtual columns. Fix this before merging.
Reviewed 90 of 90 changed files. The prior virtual-column shadowing issue is resolved.
Validation: git diff --check passed; no tests or builds were run.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 1 |
| P2 | 0 |
| P3 | 0 |
| Total | 1 |
This is an automated review by Codex GPT-5.6-Luna(max)
After addressing the findings or replying to the comments, you can request another review from me to trigger a new automated review.
| if (query instanceof WindowOperatorQuery) { | ||
| for (final OperatorFactory operator : ((WindowOperatorQuery) query).getLeafOperators()) { | ||
| if (operator instanceof ScanOperatorFactory) { | ||
| virtualColumns.addAll(List.of(((ScanOperatorFactory) operator).getVirtualColumns().getVirtualColumns())); |
There was a problem hiding this comment.
[P1] Handle null window-leaf virtual columns
Finding: WindowOperatorQuery intentionally stores null in ScanOperatorFactory when a leaf has no virtual columns. When the owning window query has a pushed-down filter, nodeFilter is non-null, so this expression dereferences that null value for any empty leaf and fails with an NPE before the node query executes.
Suggestion: Treat null leaf virtual columns as empty before adding them, and add regression coverage for a filtered window query with a leaf that has no virtual columns.
There was a problem hiding this comment.
Fixed in d4ddcf9e31.
Reproduced first: the new SystemTableQueryClientTest#testWindowLeafFilterWithoutVirtualColumns builds the window query the way production does (a filtered Scan subquery with leafOperators left for the WindowOperatorQuery constructor, which stores null virtual columns) and failed with the reported NPE at nodeVirtualColumns. nodeVirtualColumns now skips leaves whose virtual columns are null; the test asserts the filter is still pushed to the node scan with no virtual columns.
This path is reachable through a native windowOperator query without leafOperators; SQL planning does not produce it today. While adding SQL-level coverage (NativeSysServerPropertiesQueryTest#testNativeWindowFunction, both planners), a separate DECOUPLED bug surfaced: the window query is planned directly over filter(systemTable) and failed with Segment [filtered->ArrayListSegment] cannot shapeshift, because FilteredSegment never provided a CloseableShapeshifter. Fixed generally in 24dee021a5: FilteredSegment now returns CursorFactoryRowsAndColumns over its filtered cursor factory, the same way HashJoinSegment does, with a new FilteredSegmentTest. The window, DECOUPLED and Drill window SQL suites still pass.
…ries WindowOperatorQuery stores null rather than empty virtual columns for a leaf scan without any, which caused an NPE when building the node query for a filtered window query.
FilteredSegment did not provide a CloseableShapeshifter, so window queries over a FilteredDataSource failed with 'cannot shapeshift'. DECOUPLED planning produces this shape for filtered window queries on native system tables.
Follow-up
sys.tasksimplementation: FrankChen021/druid#198 (stacked on this PR).Description
This PR introduces opt-in native-query execution for supported
sys.*tables and enables it forsys.server_properties. Existing Bindable execution remains the default for compatibility.The native path is selected only when:
native."useNativeQueryForSystemTables": true.NativeSystemTable.The context parameter defaults to
false. Unsupported system tables continue to use their existing Bindable path, and other SQL engines are unaffected.Motivation
System tables traditionally use Calcite's Bindable execution path and are not represented as native Druid datasources. Consequently, native operators and aggregations are unavailable for these tables. For example:
This PR represents supported system tables as
SystemTableDataSourceinstances. Components provide their local rows through native Scan queries, after which the Broker executes the original query using Druid's native query engine.sys.server_propertiessys.tasksExecution path
The Router forwards SQL requests to a Broker normally. For component fanout, the Broker uses the standard
/druid/v2endpoint and response format rather than introducing a system-table-specific protocol.The local-route header is a routing instruction, not an authentication credential. The receiving process still applies its normal authentication and system-table authorization. It executes only the local system-table Scan, which avoids recursive Router-to-Broker or Broker-to-Broker fanout.
If the Broker itself contributes rows, it calls the raw local handler in-process with the escalator's authentication result rather than performing an HTTP loopback.
Implementation details and extension points
SQL planning
NativeSystemTablesupplies the nativeDruidTablerepresentation backed bySystemTableDataSource. Both planner strategies are supported:DruidTableScanRuleconverts the system table directly.DruidBindableTableScanRulereconstructs the native project/filter/scan plan when Calcite has already embedded filters or projections in aBindableTableScan.The DECOUPLED conversion is needed for
sys.server_properties, whose traditional table implementsProjectableFilterableTable. It runs only after native system-table planning has been selected; the Bindable path is unchanged.Registering another native system table
A new table uses the generic infrastructure and does not require a new HTTP resource, RPC client, or Broker query branch:
SystemTableDescriptorcontaining the table name, contributing node roles, row signature, routing mode, and Broker-side row authorizer.SystemTableDataProvideron the nodes that own the table's rows.NativeSystemTableand return a table backed bySystemTableDataSource.The existing
DataSourceQueryHandlerregistration then handles discovery, fanout, authorization, and component-local execution.Filter pushdown in this PR
The PR includes the generic
SystemTablePushdownFilterextraction and column-mapping mechanism. Eligible filters from the original query are copied into each component Scan, and the component handler passes provider-supported filters toSystemTableDataProvider.ServerPropertiesTableDataProvideradvertises equality pushdown forserverandservice_name. A node that does not match returns no rows before enumerating its local properties. The original filter remains on the Broker as a residual filter, so pushdown is an optimization rather than the source of final query correctness.This PR does not generate metadata SQL. Task metadata-store pushdown is part of the stacked
sys.tasksfollow-up.Native query module
NativeQueryEngineModuleis a shared facade installed by Druid server components. It combines the queryable, query-runner, segment-wrangler, joinable-factory, system-table, and query-resource modules.Its builder supports:
scanOnly()for management components that need component-local Scan execution without merge buffers or the full aggregation stack.withOverrideModule(...)for role-specific execution bindings such as the Coordinator's segment schema cache.withQueryResourceModule(...)for Broker- and Router-specific query resources.Compatibility, authorization, and current limitations
Remote requests use Druid's existing escalated HTTP client. Components authenticate them through the normal authenticator chain, and the table descriptor applies row authorization for the original user after rows return to the Broker.
useNativeQueryForSystemTablesdefaults tofalsefor rolling-upgrade compatibility. It can be enabled per query or globally on Brokers after relevant components are upgraded:druid.query.default.context.useNativeQueryForSystemTables=trueCurrent limitations:
QuerySchedulerlane and capacity limits.ScanResultValuetransport rather than frame-based exchange.Validation
Coverage includes:
COUNT(DISTINCT ...).sys.server_propertiesnode pruning.sys.server_propertiesqueries across Druid component roles.Release note
SQL queries against supported system tables can opt into Druid's native query engine with the
useNativeQueryForSystemTablesquery context parameter. The initial implementation supportssys.server_properties, uses the standard/druid/v2endpoint for component fanout, and retains the existing Bindable path by default for rolling-upgrade compatibility.This PR has: