Repository navigation
[fix](fe) Disable LOCAL TVF - #69591
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69526 Problem Summary: Add startup-only enable_local_tvf=false. Reject LOCAL TVF and FILE-local at FE admission before discovery. Explicit opt-in retains the existing storage implementation and requires ADMIN/OPERATOR. Supported explicit LOCAL sinks use the same FE admission check. ### Release note LOCAL TVF and FILE TVF with local storage require enable_local_tvf=true in fe.conf on every query-serving FE and an FE restart. This change provides default-disable mitigation; enabled storage-path sandbox hardening remains separate work. ### Check List (For Author) - Test: Focused FE unit and regression cases added but not executed. Changed Java files pass Checkstyle; diff whitespace checks pass. - Behavior changed: Yes. LOCAL access requires startup opt-in and ADMIN/OPERATOR admission. - Does this need documentation: Yes. Configuration description and release note included; user-documentation follow-up remains pending.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
Reviewed all 14 changed files at d376a62 using the supplied aggregate diff, two complete-review scopes and a separate admission-risk review. One P2 finding: the existing positive LOCAL sink binding test lacks the new startup opt-in and now throws before its assertions. No verified P0/P1 finding and no existing inline finding to carry forward. No additional user focus was supplied.
The main invocation paths checked were:
LOCAL / FILE-local SELECT, EXPLAIN, DESC
-> LocalTableValuedFunction constructor -> checkAuth/checkAccess
-> common properties -> backend discovery -> schema/scan
LOCAL INSERT / EXPLAIN INSERT
-> UnboundTVFTableSink -> BindSink.checkAccess
-> sink properties -> physical sink -> coordinator/write
Critical checkpoint conclusions:
- Goal and proof: the default-off gate covers explicit LOCAL and FILE-local query construction, plus explicit LOCAL sink analysis before destination handling. Added tests inspect disabled, enabled and denied cases. The existing positive sink fixture needs the inline fix; runtime behavior was not tested here.
- Scope and reuse: focused FE configuration/admission change; query and sink paths reuse one access routine. Deployment opt-ins and tests relate directly to the behavior change.
- Concurrency and locks: no new thread, lock, shared collection or lock order. Production reads a startup-loaded setting; no new I/O under locks.
- Lifecycle: startup loading precedes serving work. Memoized TVF objects, prepared statements and cache restrictions do not establish a same-process route around a startup-only setting. No new resource ownership or static initialization dependency.
- Configuration: enable_local_tvf defaults to false and ConfField makes it non-mutable. ConfigBase rejects runtime changes; DorisFE loads fe.conf then fe_custom.conf. Enabling it requires restarting every query-serving FE, as documented. Cloud deployment opt-ins use the existing startup configuration path.
- Compatibility: default disable is an intentional SQL behavior change described in the PR release note. Operators must opt in consistently across FEs. No incompatible function symbol, wire or storage-format change was found.
- Thrift/protobuf IDs: no gensrc changes; cross-branch field-ID checks are not applicable.
- Parallel paths: LOCAL, FILE-local, SELECT/EXPLAIN/DESC and LOCAL INSERT/EXPLAIN were traced. S3/HDFS paths remain separate. Local wildcard metadata delegation also constructs the gated LOCAL TVF. Exact-path parquet metadata is an unchanged separate API; this review does not claim a universal local-filesystem gate. Enabled storage-path containment is expressly separate PR scope.
- Conditions and planner stages: LOCAL admission follows name validation and precedes sink property checks. Existing rule registration/stage is preserved; no join, slot/ExprId, property-derivation or rewrite-loop change. Disabled and denied paths raise analysis errors.
- Test coverage: constructor tests check denial before backend/proxy interaction and enabled mocked discovery; authorization cases retain ADMIN/OPERATOR checks. Sink tests check denial and remote independence. M1 is the missed existing positive fixture. All seven supplied regression deployments opt in, so their new regression suite exercises the enabled backend-validation branch; default-off end-to-end deployment behavior and successful sink execution remain test-depth limits.
- Test results: no golden/.out result changes. The error-only regression uses test/sql/exception and requires no ordered result output. No builds or tests were run under this static-only contract.
- Observability: disabled errors name the setting and restart action; denied errors retain the existing privilege error. No new distributed operation or high-volume logging warrants added metrics here.
- Persistence and failover: no EditLog, replay, transaction-state or master-failover implementation changes. The startup option uses existing on-disk configuration loading.
- Writes and core invariants: sink rejection precedes destination side effects. Existing external-write atomicity and crash cleanup are unchanged. Committed-data visibility, visible versions, delete bitmaps and BE memory ownership are not modified by these FE admission checks.
- FE/BE variables: no added transport variable or changed serialization/send path; the gate is consumed in FE.
- Performance: constant-time configuration/privilege checks, with no added scan, copy, retry loop or large allocation; rejected requests avoid subsequent destination work.
- Other concerns and limits: the one verified finding is deduplicated across reviewers. Both initial admission risks were dismissed with concrete call-path evidence; no unresolved concrete candidate or unreviewed changed file remains. Convergence concerns this static review, not runtime validation. Builds, tests, service startup and enabled storage sandbox hardening were not performed.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: The existing positive LOCAL sink binding test reaches the new default-off gate before checking duplicate output positions. Save and enable the flag in fixture setup and restore it in teardown. Preserve the original LOCAL target, ROOT context and ExprId assertions. ### Release note None ### Check List (For Author) - Test: Changed-file Checkstyle and diff checks passed. Unit tests were not executed locally. - Behavior changed: No production behavior change; existing test fixture opts in explicitly. - Does this need documentation: No.
|
/review |
There was a problem hiding this comment.
Reviewed all 15 changed files at caacf2c728408a2686721fec39c96dad54b59f61. No new actionable findings. Existing P2 thread 4372006062 is addressed: the duplicate-output LOCAL sink fixture now saves, enables and restores the flag, retaining its ROOT context and assertions. No independently confirmed existing P0/P1 finding remains.
- Goal and evidence: The default-false gate rejects LOCAL and FILE-local reads before property parsing/backend discovery, and local sinks during binding. Added constructor/binder tests and SELECT/EXPLAIN regression cases cover admission; execution results were not verified.
- Scope and reuse: The three production changes are focused; reads and sinks share
checkAccessinstead of duplicating admission logic. - Concurrency and locking: No new runtime writer, thread or lock. Configuration loads before request serving; the flag is intentionally immutable at runtime. Test fixtures restore it, and FE test classes use separate JVM forks.
- Lifecycle and initialization: No new resource ownership, cleanup obligation or dependent static initializer. Reused TVFs still pass through existing authorization checks.
- Configuration:
@ConfFielddefaults to non-mutable;ConfigBaseloads the setting from startup configuration and rejects runtime changes. Enabling it requires configuring and restarting every FE, as described by the error and field documentation. - Compatibility: Default disable and the local sink ADMIN/OPERATOR admission are intentional behavior changes. Command-level ADMIN/LOAD checks remain. No persisted format or existing function signature changes; operators must apply the policy consistently across FEs during upgrade.
- Thrift/protobuf: No definitions changed; cross-branch field-ID checks are inapplicable.
- Parallel paths: Reviewed direct LOCAL, FILE-local delegation, SELECT/EXPLAIN/DESCRIBE, later authorization and INSERT/EXPLAIN sink binding. Remote FILE delegates and S3/HDFS sink branches avoid the new gate. Local parquet-metadata wildcard expansion also inherits admission through its existing LOCAL helper.
- Conditions and errors: Disabled admission and denied privileges fail explicitly with Nereids analysis errors. Enabled requests retain downstream property/backend validation. Planner context identity is preserved; no silent continuation was introduced.
- Test coverage: Four FE test classes cover disabled/enabled reads, CSV/Lance, FILE-local versus remote dispatch, existing ADMIN/OPERATOR/revocation behavior, and local/remote sink binding. The existing valid sink fixture covers opt-in binding; seven pipeline configs explicitly enable LOCAL. No other concrete LOCAL FE fixture requiring opt-in was found.
- Expected results: No result files changed. The error-only regression uses
test { sql; exception }; SHOW configuration returns the value at index 1, and the enabled backend-ID error occurs before file access. - Observability: The rejection explains the setting and restart requirement. Existing privilege/backend diagnostics remain; no new distributed operation needs instrumentation.
- Transactions and persistence: No EditLog, replay, failover, visible-version or delete-bitmap changes; committed-data visibility invariants are unaffected.
- Writes and atomicity: Sink admission precedes physical sink execution and coordinator/file effects. Existing external-sink transaction and atomicity behavior is unchanged.
- FE-BE variables: No new transmitted field or alternate serialization path.
- Performance and memory: Constant-time analysis checks add no per-row work, loops, large allocations or memory-ownership changes.
- Other concerns and user focus: No additional user focus was supplied. Both initial risk items were dismissed with upstream/downstream evidence; no unresolved in-scope candidate remains.
Validation was static only: no builds, tests, service startup or runtime I/O were performed. In-repository cloud/vault deployment and cloud-p1 override wiring were inspected; ordinary hosted CI orchestration is absent from this checkout and was not verified. Unrelated local-file APIs and out-of-tree plugins were outside this review's scope.
Converged after 2 review rounds, including a final review of this unchanged payload by all three assigned reviewers. No new inline comments are proposed.
Review converged after 2 round(s).
TPC-H: Total hot run time: 28513 ms |
TPC-DS: Total hot run time: 150725 ms |
ClickBench: Total hot run time: 24.45 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69526 Problem Summary: Disable LOCAL and FILE-local admission without an opt-in setting, before file discovery. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. Retain storage implementation and adapt only directly affected fixtures and suites. ### Release note LOCAL TVF and FILE TVF local storage are unavailable for all users, with no enable setting. LOCAL TVF INSERT is also unavailable. ### Check List (For Author) - Test: 13 targeted FE unit tests passed without failures/skips; changed Java Checkstyle, Groovy syntax and whitespace checks passed. Regression suite bodies not executed. - Behavior changed: Yes. Unconditional LOCAL rejection. - Does this need documentation: Yes. Release note included; operator documentation follow-up.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591, apache#69527 Problem Summary: Disable LOCAL and FILE-local admission without an opt-in setting, before file discovery. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. Retain storage implementation and adapt only directly affected fixtures and suites. ### Release note LOCAL TVF and FILE TVF local storage are unavailable for all users, with no enable setting. LOCAL TVF INSERT is also unavailable. ### Check List (For Author) - Test: 8 targeted FE unit tests passed without failures/skips; changed Java Checkstyle, Groovy syntax and whitespace checks passed. Regression suite bodies not executed. - Behavior changed: Yes. Unconditional LOCAL rejection. - Does this need documentation: Yes. Release note included; operator documentation follow-up.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591, apache#69528 Problem Summary: Disable LOCAL and FILE-local admission without an opt-in setting, before file discovery. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. Retain storage implementation and adapt only directly affected fixtures and suites. ### Release note LOCAL TVF and FILE TVF local storage are unavailable for all users, with no enable setting. LOCAL TVF INSERT is also unavailable. ### Check List (For Author) - Test: 8 targeted FE unit tests passed without failures/skips; changed Java Checkstyle, Groovy syntax and whitespace checks passed. Regression suite bodies not executed. - Behavior changed: Yes. Unconditional LOCAL rejection. - Does this need documentation: Yes. Release note included; operator documentation follow-up.
|
/review |
There was a problem hiding this comment.
Static review of head 2494b0c91859324f13bf9845dbb64eb299071377 found one new P2 regression: existing Parquet metadata local-glob tests still expect the now-disabled constructor to succeed. The unconditional LOCAL admission checks match the stated behavior; the affected metadata tests need adaptation. No P0/P1 findings.
All 41 changed paths and relevant upstream/downstream callers were covered. Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and test proof | LOCAL and FILE-local construction, DESC, explicit LOCAL INSERT and analyzed EXPLAIN reach unconditional rejection before local discovery/sink execution. Negative tests cover malformed properties, formats, privileges, delegation and sink analysis. Parsed-only EXPLAIN performs no analysis or file access. MAIN-1 identifies the missed existing regression dependency. |
| Scope and clarity | Two small production changes share one rejection method; other changes adapt dependent tests. |
| Concurrency and locking | No threads, shared mutable state or locks added; rejection precedes backend futures/coordinators. Existing lock order is unchanged. |
| Lifecycle and initialization | No new owned resources or static initialization dependencies; constructor rejection precedes resource acquisition, and test mocks are closed. |
| Configuration | No configuration added; rejection has no ADMIN/OPERATOR or startup opt-in. |
| Compatibility and upgrades | Intentional SQL feature retirement is described in the release note. No storage-format or wire compatibility changes; enforcement remains local to the FE version handling the statement. |
| Thrift/protobuf | No definitions, field IDs or enum values changed. |
| Parallel paths | LOCAL factory/Nereids, FILE-local, DESC and sink/EXPLAIN paths traced. Remote delegates remain available. The Parquet metadata glob helper is the overlooked dependency reported inline. |
| Conditional checks | Sink name is normalized before the LOCAL-only check; S3/HDFS retain their validation. Constructor rejection is unconditional. |
| Test coverage | New rejection tests and retained authorization/duplicate-output checks inspected. Remote UUID, partition and export-view branches remain reachable. MAIN-1 is the outstanding test regression. |
| Expected results | No output files changed. ORC fixture schemas/counts match destinations and existing expected counts (2/25000); value assertions and golden-tag lookup remain valid. |
| Observability | The analysis error identifies disabled LOCAL admission; existing error handling remains sufficient. No new distributed operation needs metrics. |
| Transactions/persistence/failover | No production transaction, EditLog, replay or version-visibility changes. |
| Data writes/atomicity/crashes | LOCAL sinks reject before file deletion/output execution. Remote writes are unchanged; migrated test loads use existing failure/filter/publication checks and sync. |
| FE/BE propagation | No new transmitted variables or sending paths. |
| Performance | Constant-time admission checks; no new scans, backend calls or execution-loop work. |
| Other/module invariants | No additional verified issue. FE locking/exception and Nereids registration/stage/property checkpoints checked; joins, slots, visible versions, delete bitmaps and BE memory ownership are unchanged. |
Existing thread 4372006062 is resolved at this head: the duplicate-output fixture now uses S3 and retains its ExprId assertions. It is not reposted; no existing P0/P1 finding remains to carry forward. The metadata-glob finding concerns a distinct caller and suite.
User focus: no additional focus was supplied.
Validation limits: static review only; no builds, tests or cluster operations were run. LOCAL-dependent suites and export LOCAL readback comparisons are intentionally retired, reducing that coverage. Runtime regression behavior and external service availability remain untested. No unresolved candidate or uncovered assigned static scope remains.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: Parquet metadata local wildcard expansion delegates to the disabled LOCAL TVF constructor. Expect the admission error for both the former successful glob and the no-match glob. Preserve all literal-path and remote metadata checks. ### Release note None ### Check List (For Author) - Test: Groovy 4.0.19 syntax parsing and whitespace checks passed; all other suite blocks verified byte-for-byte unchanged. Runtime regression not executed locally. No Java source changed. - Behavior changed: No. Test expectations follow the existing unconditional rejection. - Does this need documentation: No.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: Parquet metadata local wildcard expansion delegates to the disabled LOCAL TVF constructor. Expect the admission error for both the former successful glob and the no-match glob. Preserve all literal-path and remote metadata checks. ### Release note None ### Check List (For Author) - Test: Groovy 4.0.19 syntax parsing and whitespace checks passed; all other suite blocks verified byte-for-byte unchanged. Runtime regression not executed locally. No Java source changed. - Behavior changed: No. Test expectations follow the existing unconditional rejection. - Does this need documentation: No.
There was a problem hiding this comment.
Reviewed all 44 changed files at head 0977874 using static inspection. One new P2 finding: the two ORC Stream Load migrations target ARRAY/MAP columns, which the mandatory load scanner rejects before the preserved assertions. The LOCAL admission changes otherwise implement the intended unconditional shutdown.
The three existing P2 threads are addressed at this head: the duplicate-output sink fixture uses S3; matching/no-match local metadata globs expect the disabled error; the ordinary-user account cases expect that error and UUID export uses its existing S3 path. These are not reposted. No existing P0/P1 inline finding applies, so existing_blocking_comment_ids is empty. No additional user focus was supplied.
- Goal and proof: LOCAL and FILE-local construction reject before local property parsing/discovery, and explicit LOCAL sink binding rejects before sink validation. Constructor, privilege, format and INSERT/analyzed-EXPLAIN negative tests cover these entry points. The intended preservation of the two complex-data regression suites fails as described inline.
- Scope and clarity: the production change is a shared rejection called at two admission sites; accompanying test changes follow the stated feature removal. The unsupported load replacement needs correction.
- Concurrency and locking: the rejection is stateless and adds no threads, mutable shared state, locks, lock-order changes or work inside locks. Retained test uploads use unique object keys/labels.
- Lifecycle: rejection precedes local backend RPCs and sink coordinator/query registration. Existing planning cleanup and test mock teardown remain; no new ownership or initialization dependency was found.
- Configuration: no configuration is added. Shutdown is unconditional for privileged users too; dynamic configuration propagation does not apply.
- Compatibility: disabling previously supported SQL is intentional and described in the PR release note. No storage format or wire contract is changed; during a rolling FE upgrade admission follows the analyzing FE's version.
- Thrift/protobuf: no definitions, field IDs or enum values are changed.
- Parallel paths: LOCAL, FILE-local, SELECT, aliases, DESCRIBE, analyzed EXPLAIN and explicit LOCAL INSERT routes were traced. Parse-only EXPLAIN prints syntax without file access. Remote FILE/sink delegates and exact-path/remote metadata routes remain separate. Both complex ORC migrations share the reported defect.
- Conditional checks: the sink dispatch uses the already normalized function name and the shared LOCAL name. Negative expectations match the unconditional error; suite skips occur at closure entry.
- Test coverage: all six Java and 38 Groovy changes were reviewed. The PR intentionally skips 22 LOCAL-dependent suites and retires local export readback comparisons; these are coverage losses. Retained UUID S3, HDFS/S3 partition, metadata and export lifecycle cases were inspected. The two migrated ORC suites cannot provide their intended coverage until the finding is fixed.
- Test results: no .out files change. Existing 2-row/25,000-row count goldens, repeated tag order and skipped golden-block handling are compatible in isolation; the load rejection occurs before those comparisons. No builds, tests or runtime suites were executed in this review.
- Observability: the immediate admission error identifies disabled LOCAL, and skipped suites log their reason. Existing exception/status handling exposes failures; no new runtime metrics are needed for this rejection.
- Transactions and persistence: no journal, replay, visible-version or transaction implementation changes. Existing export completion/cancellation/retry checks remain; the LOCAL sink fails before execution.
- Writes, atomicity and crashes: remote write implementations are unchanged. The reported migration error is a failed load before value assertions, with no data-loss claim; no new crash-cleanup or atomicity defect was found in changed code.
- FE/BE variables: none are added. Existing Stream Load source/destination tuple serialization and scanner selection were traced to establish the finding.
- Performance: constant-time LOCAL rejection avoids property/discovery work; no new repeated scans, large allocations or hot-path loops are introduced.
- Other issues and core invariants: the final recheck covered exception types, static mocks, stale callers, removed variables, fixture schemas and scanner alternatives. No additional defect was substantiated. Committed visibility, partition versions, delete bitmaps and BE memory accounting have no changed implementation in this PR; admission failures propagate explicitly.
Review converged in round 2 after independent checks of this exact finding set and summary. Coverage is static only; runtime behavior was inferred from the traced code, and intentionally retired LOCAL success coverage is not restored by this review.
Review converged after 2 round(s).
|
run buildall |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: The ARRAY and MAP regression fixtures were migrated from LOCAL TVF to ORC Stream Load, but the load scanner rejects complex destination columns. Upload the original fixtures to unique temporary S3 objects and use INSERT SELECT from S3 TVF, cleaning up each object after the insert. Preserve table definitions, synchronization and all existing count/value checks. ### Release note None ### Check List (For Author) - Test: Groovy 4.0.19 syntax parsing, ORC fixture schema/row-count/content inspection and git diff --check passed. Runtime regression was not executed locally; matching FE/BE artifacts are unavailable in these task worktrees. - Behavior changed: No (test data-loading adaptation only) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Static review of head 215f8dd covered all 44 changed files (6 Java and 38 Groovy), related invocation paths, and the supplied existing threads. One new P2 finding concerns Azure compatibility of the two migrated ORC fixture uploads. No new P0/P1 issue was verified.
Critical checkpoint conclusions:
- Goal and evidence: LOCAL source construction and explicit LOCAL sink analysis reject unconditionally; FILE(local), SELECT/EXPLAIN, and analyzed INSERT/EXPLAIN reach these gates. Source/sink unit tests and account regression assertions cover early rejection and retained remote validation; inspected only.
- Scope: production edits are small and centralized. Regression adaptations follow the intentional LOCAL disablement; the fixture-provider regression is reported inline.
- Concurrency and locks: no production shared state, threads, locks, or lock ordering changes. Test static mocks are scoped and closed.
- Lifecycle and initialization: source rejection precedes backend discovery; sink rejection precedes writer/coordinator setup. ORC fixture objects are deleted in finally after synchronous INSERT completion. No new static initialization dependency.
- Configuration: no admission flag or other production configuration is added. Existing Azure regression configuration exposes the reported upload-client mismatch.
- Compatibility: LOCAL SQL behavior intentionally changes as documented in the release note; wire layouts, persisted formats, and function signatures are unchanged.
- Thrift/protobuf: no definitions or IDs changed.
- Parallel paths: checked direct LOCAL, FILE delegation, registration/DESC construction, sink INSERT/analyzed EXPLAIN, and metadata wildcard expansion. Remote source/sink branches remain outside the new rejection. Exact-path metadata queries bypass wildcard expansion.
- Conditional checks: the added sink condition uses the normalized function name, and the shared rejection documents its early-failure purpose. Existing command-level INSERT privileges can still reject before planning.
- Test coverage: new negative unit and account tests cover malformed properties, formats, privilege values, no backend interaction, and remote validation. Existing LOCAL-only success suites are intentionally skipped; local export readback coverage is retired while export completion/cancellation and surviving remote cases remain. Runtime/build validation was not performed under this static-only contract.
- Results: no .out files changed. Framework tag lookup tolerates retired tags; fixture schemas and row counts match the explicit ORC projections and unchanged count results (2 array rows; 25,000 map rows). Ordered array-value checks remain.
- Observability: disabled statements return an explicit analysis error; suite skip logging and export-state/error checks remain. No added distributed operation needs new metrics.
- Transactions and persistence: no EditLog, replay, visible-version, delete-bitmap, or transaction lifecycle change.
- Writes and atomicity: LOCAL fails before file writing/deletion. Existing remote execution is unchanged; fixture upload compatibility is the reported test failure, with no verified data-visibility or atomicity regression.
- FE/BE variables: no new transmitted variables or serialization paths.
- Performance and memory: the gate is constant-time and avoids LOCAL discovery; no new hot-path scans, large allocations, or BE memory-ownership changes.
- Additional review: checked wrapper exception propagation, indirect constructor users, retained test branches, and duplicate-output ExprIds. No additional verified issue or unresolved static code question remains.
Existing findings: all four supplied P2 threads are addressed at this head: the sink binding fixture uses S3; metadata wildcard cases expect disablement; account errors and UUID export use the updated paths; complex ORC fixtures load through S3 TVF rather than unsupported Stream Load. The new Azure-provider finding is distinct from that earlier ORC-format issue. No existing P0/P1 finding needs carrying forward, and no duplicate inline finding is posted.
User focus: none supplied beyond a complete review. Coverage limit: conclusions are based on static source/diff and fixture inspection; no builds, unit tests, regression suites, or live provider operations were executed.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: The migrated ARRAY and MAP fixtures always used an AWS client even when the regression provider was Azure. Select the existing Azure Blob SDK for Azure upload/cleanup and pass the selected provider and endpoint to S3 TVF. Resolve the framework Azure endpoint placeholder while preserving explicit endpoints. Keep S3 uploads, original fixtures and all existing assertions. ### Release note None ### Check List (For Author) - Test: Eight scripts compiled with the existing SDK dependencies; 12 loopback SDK upload/cleanup scenarios passed, including mixed-case Azure, default/explicit endpoints and injected SQL failure. SQL was intercepted in this probe; no Doris runtime regression or live cloud validation was performed. git diff --check passed. - Behavior changed: No (test data-loading adaptation only) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Static review of all 44 changed files at 30ce3e7 found one new P2 regression: the three UUID fixture suites that now always upload with the AWS client lose their previously working Azure-plus-local-fixture configuration. The inline comment identifies that precise configuration; the already-broken Azure fallback without local overrides is not claimed as new.
Critical checkpoint conclusions:
- Goal and evidence: LOCAL construction and explicit LOCAL sink binding now reject admission unconditionally. Added FE tests cover rejection before file access/property validation, privileged users, FILE local dispatch, INSERT and analyzed EXPLAIN, and unchanged remote validation. Tests were inspected, not executed.
- Scope and clarity: production changes are two small edits using one stateless rejection helper. Most changed files adapt tests to deliberate LOCAL removal.
- Concurrency and locks: no production threads, shared mutable state, locks or lock-order changes. Fixture object names are unique.
- Lifecycle and initialization: rejection precedes backend/RPC resource acquisition. Test static mocks close after each case; ORC fixture cleanup follows synchronous INSERT in finally. No new static-initialization dependency.
- Configuration: no new configuration or opt-in; the intended rejection is unconditional.
- Compatibility: LOCAL reads/writes are intentionally disabled, including ADMIN/OPERATOR. No storage-format or wire-symbol change requires rolling-upgrade adaptation; the PR release note describes the behavior change.
- Thrift/protobuf: no definitions changed.
- Parallel paths: direct LOCAL, FILE with local storage, and explicit LOCAL sinks reach rejection. Remote FILE delegates and S3/HDFS sinks remain separate. Local metadata wildcard expansion also rejects through LOCAL; explicit-path metadata remains separate.
- Conditional checks: the sink name is normalized before the local-only check; placement is documented and precedes sink-property validation. Error propagation preserves the disabled message.
- Test coverage: all four Java test files and all 38 Groovy changes were reviewed. Local-only suites intentionally return; mixed suites retain remote checks. The new P2 concerns three different UUID loaders from the previously fixed ORC loaders.
- Expected results: no golden files changed. ORC footer field names/counts match the new projections and existing results (2 array rows, 25000 map rows); replacement COUNT queries preserve tags/values. The framework skips retired golden blocks by tag.
- Observability: existing analysis/query errors expose rejection; no new distributed operation needs additional production metrics or logging.
- Transactions and persistence: no journal, replay, failover or persisted metadata logic changed.
- Writes and atomicity: LOCAL sinks fail before physical sink/coordinator execution. Production remote writes and atomicity are unchanged; fixture upload failures remain visible to the test framework.
- FE/BE variables: no transmitted field or session variable added.
- Performance and memory: the new sink check is constant-time; rejected LOCAL avoids property parsing and backend discovery. No new large allocation or repeated scan; visibility, delete-bitmap and memory-tracking mechanisms are unchanged.
- Additional issues: the final missed-issue checks covered surviving callers, exception families, analyzed EXPLAIN, provider routing, cleanup, fixture schemas and output alignment; no second verified new defect was found.
All five existing P2 thread subjects were revalidated as addressed at this head: the sink binding fixture, local metadata glob expectations, account/UUID-export adaptation, complex-ORC loading, and Azure upload for those two ORC loaders. They are not reposted. No existing P0/P1 finding was present, so existing_blocking_comment_ids is empty. No additional user focus was supplied.
Coverage limits: static analysis only, as required. No builds, tests, cluster execution, SDK loopback probes or live-cloud validation were performed in this invocation. Local-only success/readback coverage is deliberately retired by this PR. There are no unresolved code candidates or assigned coverage gaps after the final reviewer pass.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: Removing the disabled LOCAL fallback makes three UUID fixture suites use an AWS upload even for Azure. Use the already-declared Azure Blob SDK for Azure uploads and pass the matching provider/endpoint to TVF. Preserve explicit endpoints, the existing S3 upload path, original fixtures and all schema/value/profile assertions. This change is limited to these three existing suites. ### Release note None ### Check List (For Author) - Test: Six scripts compiled with existing SDK dependencies. Twelve loopback SDK scenarios verified the three exact UUID setup/upload blocks, Azure default/explicit endpoints, S3 uploads, byte-identical fixtures, matching TVF parameters and fixture availability for later queries. No SQL execution, Doris runtime regression or live cloud validation was performed. git diff --check passed. - Behavior changed: No (test data-loading adaptation only) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Static review of all 44 changed files at ef4eca4 found one new P2 regression: UUID EXPORT/readback still lacks Azure provider/endpoint adaptation after removal of its LOCAL override. This is a separate SQL export path from the fixture uploaders in the existing Azure discussions.
Critical checkpoint conclusions:
- Goal and evidence: unconditional LOCAL reads, FILE-local reads and LOCAL sink binding reject before local discovery or sink setup. New FE and account tests cover rejection, privileged users, EXPLAIN and preserved remote validation; inspected statically only.
- Scope and reuse: the two production edits share one rejection method. Regression changes retire LOCAL-dependent success paths and retain independent remote cases.
- Concurrency and locks: no new threads, shared mutable state, locks or lock ordering; rejection precedes local RPC and coordinator work.
- Lifecycle and static initialization: no new static initialization dependency. Static mocks close after each test; array/map fixture objects are deleted in finally after synchronous INSERT completes.
- Configuration: no new setting or enable flag; dynamic propagation is inapplicable.
- Compatibility: disabling LOCAL SQL is intentional and release-noted. No incompatible serialized format or FE-BE protocol change was found; remote delegates remain available.
- Thrift/protobuf: no definitions changed; cross-branch field-id checks are inapplicable.
- Parallel paths: inspected direct LOCAL, FILE delegation, TVF factories, SELECT/INSERT and analyzed EXPLAIN, metadata wildcard expansion, and unchanged callers. The remaining provider failure is reported inline.
- Conditions and errors: normalized sink-name selection gates LOCAL before sink property validation. Constructor rejection reaches existing analysis-error reporting. Earlier INSERT privilege rejection remains unchanged.
- Test coverage: reviewed all four changed FE test classes and all 38 regression scripts. Negative tests use exception assertions. Retained remote schema/value/profile assertions remain; LOCAL content, cache and roundtrip coverage is deliberately retired.
- Expected results: no golden files changed. The framework skips unused retired tags; replacement array/map count checks match existing values of 2 and 25000, and original fixtures/schemas are preserved.
- Observability: the disabled message and skip logs explain the changed behavior; no new distributed operation requires metrics or additional tracing.
- Transactions and persistence: no EditLog, replay, visible-version, delete-bitmap or failover behavior changes.
- Writes and atomicity: rejected LOCAL sinks stop during analysis before deletion, query registration or BE execution. Remote fixture lifetimes cover synchronous loads; no additional cleanup or atomicity defect was substantiated.
- FE-BE variables: no new transmitted variables or fields.
- Performance and memory: production rejection is constant-time and avoids discovery work; no new BE allocation or unbounded scan/retry pattern was introduced. Fixture uploads are a fixed finite set with unique keys.
- Other issues: further sweeps found no additional actionable issue or unresolved static candidate. No additional user focus was supplied.
Existing threads were independently revalidated and not reposted: the sink fixture now uses S3; metadata/account cases expect rejection; ORC complex-column fixtures use remote TVF; the five fixture uploaders select Azure where needed. The UUID export provider instance reported here remains distinct. No existing P0/P1 inline finding requires retention.
Limits: review was static only; no builds, tests, SQL, cluster or live-cloud validation were performed. Deliberately retired LOCAL success/readback coverage is not replaced by runtime validation in this review.
Review converged after 2 round(s).
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69591 Problem Summary: UUID export/readback forwards the Azure endpoint placeholder without its provider after the LOCAL fallback is removed. Resolve the Azure endpoint and include provider in the existing shared properties used by both EXPORT and the readback TVF. Keep all fixture data, export state/error handling and ordered result assertions unchanged. ### Release note None ### Check List (For Author) - Test: Both Groovy scripts compiled. Eight intercepted-SQL scenarios verified shared export/readback properties for Azure default/explicit endpoints and OSS, retained export-state handling and ordered golden-check invocations. No Doris SQL execution or live storage validation was performed. git diff --check passed. - Behavior changed: No (test data-loading adaptation only) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
Reviewed exact head d197857e663ea16cac712e6b2081b2650d8d5c6b. No new actionable findings. All seven existing P2 reports were independently revalidated and are addressed in the current code; there are no existing P0/P1 findings to carry forward.
Coverage includes all 44 changed files (six Java and 38 Groovy), related analyzer/storage/export paths, regression framework behavior and existing golden results. Two complete-scope reviewers and a separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; all initial risk questions have evidence-backed dispositions.
Critical checkpoint conclusions:
- Goal and evidence: LOCAL construction rejects before authorization, LOCAL property parsing, backend selection and file discovery. FILE-local delegates to that constructor. Explicit LOCAL INSERT and analyzed EXPLAIN reach the sink-binding rejection before sink-property validation or execution. Inspected FE tests and account regressions encode these negatives and remote controls.
- Scope and reuse: the production change is two small gates sharing one rejection method. The broader test changes adapt consumers to the intentional withdrawal of LOCAL support.
- Concurrency and locking: no new threads, shared mutable production state, locks, lock ordering or blocking operations are introduced.
- Lifecycle and initialization: no production ownership or static-initialization change. Unit-test static mocks close in teardown; migrated ORC fixtures use unique object keys and matching-provider cleanup in finally. UUID remote-fixture lifetime follows the existing remote branch.
- Configuration: no enable flag or dynamic opt-in is added; rejection is unconditional.
- Compatibility: this intentionally changes SQL behavior for LOCAL, FILE-local and explicit LOCAL sinks for all users, as stated in the release note. Storage formats and serialized state are unchanged.
- Thrift/protobuf: no definitions change; field-ID and rolling-protocol checks are inapplicable.
- Parallel paths and stage order: direct LOCAL, FILE-local, ordinary INSERT and analyzed EXPLAIN reach the intended gates. Remote FILE dispatch and S3/HDFS sink validation remain separate. Metadata local wildcard expectations are adapted; its independent exact-path and remote cases remain.
- Conditions and errors: the new sink-name condition selects only LOCAL and raises the shared explicit analysis error. Existing expression/statement exception boundaries preserve failure and its message; no silent continuation is added.
- Test coverage: inspected tests cover malformed/missing LOCAL properties, multiple formats, privileged rejection, FILE delegation, remote controls and distinct sink output positions. LOCAL-dependent success suites and local export-readback comparisons are deliberately retired; retained export completion/error, remote partition, UUID and ORC value checks remain in source.
- Expected results: no .out files change. The framework skips retired golden tags. ORC fixture footer column names and row counts match the new explicit projections and existing first golden counts (2 and 25,000); subsequent value assertions remain.
- Observability: the stable disabled analysis error is sufficient for deterministic admission rejection; no new operational logs or metrics are needed.
- Persistence and failover: no EditLog, replay, persisted metadata or transaction failover logic changes.
- Writes and atomicity: LOCAL sinks reject before execution setup. Remote write/transaction implementations are unchanged; test fixture deletion follows synchronous INSERT completion or failure.
- FE/BE variables: no new fields or transmission paths are introduced. Provider/endpoint/credential identity in the migrated fixtures and UUID export/reload is preserved through the existing storage-property path.
- Performance: rejection is constant-time and avoids later LOCAL parsing/discovery work; no new production hot-path scan or allocation is introduced.
- Other concerns and user focus: no additional user focus was supplied and no unresolved code candidate remains. Earlier sink-test, metadata-glob, account/UUID, complex-ORC and Azure-routing findings are fixed in their existing instances and are not reposted.
Validation limits: this was static review only. No builds, tests, Groovy compilation, Doris SQL execution or live Azure/S3 validation were performed. Retired LOCAL success/readback coverage is not claimed as retained runtime coverage. The review converged in one round.
Review converged after 1 round(s).
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28598 ms |
TPC-DS: Total hot run time: 151064 ms |
|
run external |
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 28776 ms |
TPC-DS: Total hot run time: 150575 ms |
ClickBench: Total hot run time: 24.49 s |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: N/A Related PR: #69591, #69527 Problem Summary: LOCAL TVF can discover files on a backend during analysis. Reject LOCAL unconditionally before property parsing, backend selection and file discovery. FILE TVF with local storage delegates to the same entry point. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. No configuration option enables these paths, including for ADMIN/OPERATOR. The underlying storage implementation is retained. Tests assert rejection before backend access and preserve remote FILE/sink behavior where supported. Existing LOCAL-dependent success suites explicitly return while the feature is unavailable. Export completion/error checks remain active; LOCAL export-readback comparisons are retired. Remote HDFS/S3 partition cases, the existing UUID S3 fixture branch, and ORC value checks through S3 TVF using the original uploaded fixtures remain active where present on this branch. ### Release note LOCAL TVF and FILE TVF with local storage are disabled for all users. There is no configuration option to re-enable them. Explicit LOCAL TVF INSERT is also disabled. ### Check List (For Author) - Test - [x] Unit Test — 8 targeted FE tests passed; zero failures, errors or skips. - [x] Regression test — rejection coverage and affected-suite adaptations included; suite bodies have not been executed locally. - [ ] Manual test - [ ] No need to test Validation: Checkstyle passed for all 5 changed Java files. Changed Groovy suites passed syntax parsing with Groovy 4.0.19. Whitespace checks passed. Runtime regression and formal local code review have not been performed. - Behavior changed: - [ ] No. - [x] Yes. LOCAL admission always fails; no startup opt-in is available. - Does this need documentation? - [ ] No. - [x] Yes. Release note included; operator documentation should reflect disabled LOCAL support. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
### What problem does this PR solve? Issue Number: N/A Related PR: #69591, #69528 Problem Summary: LOCAL TVF can discover files on a backend during analysis. Reject LOCAL unconditionally before property parsing, backend selection and file discovery. FILE TVF with local storage delegates to the same entry point. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. No configuration option enables these paths, including for ADMIN/OPERATOR. The underlying storage implementation is retained. Tests assert rejection before backend access and preserve remote FILE/sink behavior where supported. Existing LOCAL-dependent success suites explicitly return while the feature is unavailable. Export completion/error checks remain active; LOCAL export-readback comparisons are retired. Remote HDFS/S3 partition cases, the existing UUID S3 fixture branch, and ORC value checks through S3 TVF using the original uploaded fixtures remain active where present on this branch. ### Release note LOCAL TVF and FILE TVF with local storage are disabled for all users. There is no configuration option to re-enable them. Explicit LOCAL TVF INSERT is also disabled. ### Check List (For Author) - Test - [x] Unit Test — 8 targeted FE tests passed; zero failures, errors or skips. - [x] Regression test — rejection coverage and affected-suite adaptations included; suite bodies have not been executed locally. - [ ] Manual test - [ ] No need to test Validation: Checkstyle passed for all 5 changed Java files. Changed Groovy suites passed syntax parsing with Groovy 4.0.19. Whitespace checks passed. Runtime regression and formal local code review have not been performed. - Behavior changed: - [ ] No. - [x] Yes. LOCAL admission always fails; no startup opt-in is available. - Does this need documentation? - [ ] No. - [x] Yes. Release note included; operator documentation should reflect disabled LOCAL support. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
What problem does this PR solve?
Issue Number: N/A
Related PR: #69526
Problem Summary:
LOCAL TVF can discover files on a backend during analysis. Reject LOCAL unconditionally before property parsing, backend selection and file discovery. FILE TVF with local storage delegates to the same entry point. Explicit LOCAL INSERT and analyzed EXPLAIN INSERT use the same rejection before sink property validation. No configuration option enables these paths, including for ADMIN/OPERATOR. The underlying storage implementation is retained.
Tests assert rejection before backend access and preserve remote FILE/sink behavior where supported. Existing LOCAL-dependent success suites explicitly return while the feature is unavailable. Export completion/error checks remain active; LOCAL export-readback comparisons are retired. Remote HDFS/S3 partition cases, the existing UUID S3 fixture branch, and ORC value checks through S3 TVF using the original uploaded fixtures remain active where present on this branch.
Release note
LOCAL TVF and FILE TVF with local storage are disabled for all users. There is no configuration option to re-enable them. Explicit LOCAL TVF INSERT is also disabled.
Check List (For Author)
Validation: Checkstyle passed for all 6 changed Java files. Changed Groovy suites passed syntax parsing with Groovy 4.0.19. Whitespace checks passed. Runtime regression and formal local code review have not been performed.
Check List (For Reviewer who merge this PR)