Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Two verified findings require changes: [P1] signed-zero port syntax still lets S3 Lance bypass the new LOCAL admission policy through the SDK's local-path fallback; [P2] LOCAL Lance file-URI canonicalization changes percent-encoded dataset identity. Both are reported inline. There were no existing inline threads or blocking comment IDs, and no additional user-provided focus points.

All 20 changed files were reviewed, with FE, BE and independent boundary-risk coverage. The complete LOCAL/FILE/S3 entrypoint -> property normalization -> schema/metadata discovery -> scan-range -> dataset-open/cache chain was traced against this head and the pinned SDK sources.

Individual critical-checkpoint conclusions:

1. Goal and proof: default-disable, early/repeated privileges and canonical glob containment are implemented, but the two findings leave security and URI compatibility incomplete. Added tests exercise the intended paths; they miss signed-zero ports and escaped file-URI collisions.
2. Scope: production edits are focused on admission, canonicalization and Lance identity, with corresponding tests and regression opt-ins.
3. Concurrency and locks: no new threads, shared mutable state or lock order; existing heavy-work RPC/scanner contexts perform filesystem work without newly introduced critical sections.
4. Lifecycle/static initialization: existing RAII, close paths, per-reader dataset ownership and lazy Session initialization remain intact; no new cross-TU static dependency or ownership cycle.
5. Configuration: enable_local_tvf defaults false and is startup-only because ConfField.mutable defaults false and setMutableConfig rejects it. Every serving FE must opt in and restart, as described.
6. Compatibility: no storage/wire format change; legacy scan-level file-type fallback and positive-version catalog behavior remain. Encoded file-URI compatibility regresses as reported.
7. Thrift/protobuf: no definitions changed, so release-branch ID comparison is not applicable.
8. Parallel paths: LOCAL and FILE-local share constructor/authorization guards; schema and execution use FILE_LOCAL/version zero; S3 and catalog snapshots use fixed positive versions. The S3 parser bypass remains actionable.
9. Conditions: range-type precedence and version-zero exceptions are documented and backed by actual producer invariants; no user TVF option supplies a positive local version.
10. Test coverage: inspected FE positive/negative admission and URI tests, BE glob/root/symlink/schema/relative-scan tests, and SELECT/EXPLAIN Groovy gate checks. Add cases for both inline findings. No build or test was run under this static-only contract.
11. Expected results: no generated output snapshots changed; the new four-row expectation matches the existing Lance fixture usage. The Groovy suite checks gate/backend rejection, not a successful end-to-end read.
12. Observability: existing profile/error paths remain; generic containment errors intentionally redact expanded/resolved target names. No new critical metrics requirement identified for admission-only work.
13. Transactions/persistence: no EditLog, transaction, visible-version, delete-bitmap or master-failover behavior changes.
14. Writes/atomicity/crashes: production changes are read-only; no new data-write transaction or persistent cleanup state.
15. FE/BE variables: an existing file_type field is now retained on Lance TVF execution ranges; schema already tags it and legacy scan-level handling is preserved.
16. Performance/memory: validation is per TVF/schema/split rather than per row; no material new allocation, cache or hot-loop regression substantiated. Glob root recanonicalization is bounded per match.
17. Further recheck: candidate triggers, upstream guarantees, downstream SDK dispatch, tests and all changed paths were rechecked; no other introduced actionable defect was verified.

Review limits: static source review only; author-reported runs were not independently executed, and actual remote reads, Groovy integration and mixed-version runtime behavior remain untested here. Pre-existing descendant-symlink/manifest child confinement and filesystem-mutation races were examined and excluded from new findings because this PR did not introduce or expand them. No unresolved review candidate or uncovered changed-file scope remains.

Review converged after 2 round(s).

One verified P2 correctness regression remains: a LOCAL Lance file URI containing a symlink followed by .. can select a different dataset inside the secure root. The inline finding gives the exact path, SDK behavior, and fix direction. No additional P0/P1 finding was verified.

Reviewed head 398acc158bbb9e2a9525a0960a9186f351d73fc6 across all 22 changed paths and related callers. Read flow: LOCAL/FILE -> opt-in and read privileges -> properties/backend -> glob or schema RPC -> tagged Lance range -> secure resolver -> dataset open. Write flow: INSERT -> existing write authorization -> sink binding/LOCAL opt-in -> execution; EXPLAIN shares the sink-binding gate.

Critical checkpoint conclusions:

• Goal and data correctness: default opt-in, early access checks, glob containment, and remote Lance admission are implemented; the inline URI-normalization defect prevents complete path-identity preservation.
• Scope and clarity: changes stay within admission, path handling, range identity, tests, and regression configuration; no unrelated rewrite was identified.
• Concurrency and locking: no new threads, production shared mutable state, locks, or lock order. Glob retains its existing heavy-work-pool execution.
• Lifecycle and initialization: validation precedes schema/cached opens; existing dataset/session ownership remains intact, with no new cross-translation-unit static dependency.
• Configuration: enable_local_tvf defaults false and is intentionally immutable at runtime. Each query-serving FE must be configured and restarted. Seven regression deployments explicitly opt in.
• Compatibility: default disablement is an intentional documented change. No storage format or function signature changes; fixed-version catalog warehouses retain their routing. File-URI compatibility has the reported regression.
• Thrift/protobuf definitions: none changed, so cross-release ID/enum comparison is inapplicable.
• Parallel paths: checked LOCAL and FILE-local, SELECT and EXPLAIN, INSERT and EXPLAIN INSERT, remote sinks, schema and scan, shared storage, legacy scan tags, and fixed-version catalogs. Dependent metadata glob discovery inherits LOCAL admission; the flag is not a blanket policy for every local-file feature.
• Special conditions: version-zero LOCAL ranges receive containment checks; positive catalog snapshots retain operator-managed locations; per-range file type overrides legacy scan-level type. Remote scheme/port conditions have explicit rationale.
• Test coverage: inspected enabled/disabled admission, privilege revocation, FILE delegation, sink binding, remote normalization/Broker rejection, glob traversal/symlink/root variants, escaped URI identity, schema/scan, and catalog exceptions. The symlink-plus-URI-dot-segment case is missing. Real remote reads and full integration execution are not proven by mocked unit tests.
• Expected results: no golden output files changed; no incorrect added expected result was identified statically.
• Observability: admission errors identify configuration or privilege requirements; containment errors intentionally hide expanded names. Existing Lance URI/version profiles and error propagation remain sufficient for these setup paths.
• Transactions and persistence: no transaction, EditLog, persisted metadata, visibility-version, or delete-bitmap logic changed.
• Data writes and crash behavior: write changes add an admission check before execution; existing atomicity, cleanup, and crash behavior are not altered.
• FE/BE propagation: no new wire variable; scan ranges now preserve the existing file_type field, as schema requests already do. Both local schema and execution use the same resolver.
• Performance and memory: new work occurs at glob/dataset setup rather than per row; no new large allocation or ownership path was identified.
• Other issues and user focus: no additional user focus was supplied; complete and risk-focused rechecks found no further verified defect.

Existing findings were independently revalidated: signed-zero ports, 4361447979 are now rejected by raw ASCII-digit validation, and escaped file URIs, 4361447989 now decode once with collision coverage. Neither original instance still applies; neither is reposted, and there are no confirmed existing P0/P1 blockers.

Static review only: no builds, tests, source modifications, or submodule initialization were performed. All assigned scopes and candidates have explicit dispositions. The review converged in round 2 after all three reviewers rechecked this exact final payload and returned NO_NEW_VALUABLE_FINDINGS. Runtime validation and the missing regression case remain testing limits, not unresolved review candidates.

Review converged after 2 round(s).

Static review of PR #69528 at 398acc1. No new actionable findings were verified. The review converged after one round of complete BE/FE review and a separate cross-layer risk review.

Existing review disposition:

• The dot-segment/symlink dataset-identity issue in comment 4365938520 still applies: file-URI path segments reach filesystem canonicalization before URL lexical normalization. It is already reported and is not duplicated here.
• The percent-decoding issue in comment 4361447989 is fixed for the reported escaped-name collision: actual file: paths are decoded once, bare percent paths stay literal, and both schema discovery and scanning use the canonical result.
• The signed-zero port P1 in comment 4361447979 is fixed by checking the original port characters before metadata loading. No previously reported P0/P1 was confirmed to remain applicable, so existing_blocking_comment_ids is empty.

Critical checkpoints:

1. Goal and behavior: the default-off LOCAL switch guards LOCAL/FILE-local reads before property parsing or backend access and independently guards INSERT/EXPLAIN sink binding. Canonical containment covers glob matches and LOCAL Lance schema/scan opens; the existing URI normalization P2 remains as noted above.
2. Scope and clarity: changes are confined to admission, URI/path validation, existing range-type propagation, focused tests, and pipeline opt-ins.
3. Concurrency and locks: no new threads or lock acquisition/order. New path values are request-local; shared Lance session/cache synchronization is unchanged.
4. Lifecycle/static initialization: existing schema/dataset ownership and cleanup remain; no new cross-translation-unit initializer dependency or ownership cycle.
5. Configuration: enable_local_tvf is immutable and false by default; the error documents restarting every FE. Cloud and non-cloud FE use the same check, and seven test pipeline configs explicitly opt in.
6. Compatibility: no wire or storage-format definition changes. The existing optional range file_type is now set explicitly; the BE retains scan-level fallback. Positive-version catalog scans preserve their configured URI. Older FEs do not gain the new admission policy during a mixed-version rollout.
7. Thrift/proto: none modified; cross-release field/enum comparison is inapplicable.
8. Parallel paths: checked LOCAL/FILE aliases, regular glob/direct Lance reads, schema/scan/cached opens, INSERT/EXPLAIN, catalog/TVF, and remote/local range precedence.
9. Conditions: comments explain explicit-range precedence and the version-zero LOCAL versus positive-version catalog distinction. Final remote Lance validation runs after storage normalization, including Broker passthrough.
10. Test coverage: added FE tests cover gate/auth ordering, aliases, sink binding, storage types and malformed/valid remote URI cases. Added BE tests cover canonical containment, glob-root literals, symlink escape rejection, relative Lance reads and escaped dataset-name collisions. The regression suite checks admission in both configuration states. The existing dot-segment collision lacks a regression case in this patch.
11. Expected results: inspected assertions check canonical keys, schema discovery, scan row IDs, explicit failures and absence of premature backend/metadata calls; no golden output files changed. Tests were not executed.
12. Observability/errors: validation failures propagate explicitly; expanded out-of-root names are hidden. Existing Lance open timers, URI/version profiles and session metrics remain. No new distributed operation needs separate instrumentation.
13. Transactions/persistence: no EditLog, transaction, visibility or failover-persistence logic changed.
14. Writes/atomicity: the patch changes sink admission; existing ADMIN/global-LOAD execution authorization and writer/commit behavior remain. No new crash-cleanup path is introduced.
15. FE/BE propagation: each Lance TVF execution range carries the TVF's existing storage type, while schema requests already carry it; no new transmitted variable.
16. Performance: path resolution occurs per path/split/open, outside row/batch decoding. No new per-row allocation, lock-heavy work, or change to pruning/materialization was found.
17. Other core invariants: no changed MoW/delete-bitmap, nullable-column, decoder, or committed-data visibility behavior. Further parser/cache/admission and SDK-provider rechecks produced no additional verified issue.

There was no additional user-provided focus. All changed paths and the three initial risk areas were covered and given a disposition; no unresolved candidate or uncovered changed scope remains. This was a static-only review: no builds, tests, runtime parser experiments, source changes, or submodule initialization were performed. Convergence means the review found no additional valuable issue after rechecking; it does not resolve the pre-existing P2 thread above.

Review converged after 1 round(s).

Static review of all 22 changed paths at 71be77f48222334cf3311ed71ba9f4b6142043e4: one new P2 finding, inline below. A file: URI with raw trailing whitespace can select the wrong in-root Lance dataset. No new P0/P1 finding was verified.

The existing signed-zero-port finding (4361447979) and percent-decoding finding (4361447989) are fixed for their reported scenarios. The reported file-URI example in dot/symlink thread 4365938520 is fixed, but its bare absolute-path variant remains: the SDK lexically removes jump/.. before following symlinks, while the new canonicalizer follows the symlink first. This is the same already-reported mechanism/location and is not reposted. The pinned path_abs implementation establishes that bare-path baseline. No existing P0/P1 remains applicable. There were no additional user-provided focus points.

Individual critical-checkpoint conclusions:

• Goal and proof: LOCAL/local FILE reads and LOCAL sinks now require opt-in; read authorization precedes backend access. Added tests cover admission and secure-path behavior, but omit the whitespace form in the new finding and its rootless-URI metadata variant.
• Scope and clarity: the change is focused on admission, remote-URI validation, Lance range identity, canonical containment, and supporting tests/configuration.
• Concurrency and locks: statement-local guards and path buffers add no shared mutable state or locks. Glob/schema work uses the existing worker pool; no new lock-order or heavy-under-lock issue was found.
• Lifecycle and initialization: schema handles retain RAII cleanup; reader-owned datasets and split-owned scanners keep their existing close order. No new cycle, escaping reference, or cross-unit static dependency was found.
• Configuration: enable_local_tvf defaults false and is nonmutable; the documented every-FE restart matches ConfigBase enforcement. Seven regression pipelines explicitly opt in.
• Compatibility: no storage-format or function-signature change. Existing range tags and legacy scan-level fallback are handled; positive catalog snapshots retain their operator-configured URI. The new finding and the already-known bare-dot variant are verified input-compatibility defects. A deployed rolling-upgrade matrix was not executed.
• Thrift/protobuf: no definition, ID, enum, or required-field change; release-branch ID comparison is inapplicable.
• Parallel paths: direct LOCAL, delegated FILE, INSERT/EXPLAIN, schema/scan/cache opens, remote providers, and fixed-version local catalogs were traced. The sink retains its existing ADMIN/LOAD execution check.
• Special conditions: comments and producers support the LOCAL-plus-version-zero TVF distinction and per-range precedence. Remote validation checks the final normalized URI, including Broker passthrough.
• Test coverage: FE tests inspect rejection before I/O, privilege changes, sink analysis and range tags; BE tests cover canonical roots, glob escapes, URI decoding/dots and real schema/row expectations. The account regression covers configured admission. Whitespace-URI scan collisions and rootless-URI DESCRIBE collisions are missing; bare dot/symlink controls need a differential check against the SDK baseline.
• Expected results: no .out artifact changed; new assertions match the inspected branches, but bare dot/symlink expectations also need alignment with the SDK baseline noted above. Tests were inspected, not run.
• Observability: admission errors name the control/privilege; failed glob containment hides expanded external names; existing dataset/version errors, timers and profile strings remain available.
• Transactions and persistence: no EditLog, commit/publish, visible-version, delete-bitmap, persisted-metadata, or failover-state change.
• Writes and crash behavior: the added sink gate runs before backend scheduling. No writer, atomicity, transaction or durable-resource lifecycle change was introduced.
• FE-to-BE passing: no new variable; existing file_type is now explicit for Lance scan ranges as it already was for schema requests. Both producers and legacy fallback were checked.
• Performance and memory: added URI work is bounded by path length and occurs outside row loops. No significant retained allocation or material hot-path regression was identified; no benchmark was run.
• Other invariants: no nullable-column/materialization change or new version/delete-bitmap handling; failures propagate before opening. The final missed-path sweep found no additional independent issue beyond the new parser finding and deduplicated historical variant.

Coverage limit: this was a static-only review, as required. No build, test, sanitizer, live-storage or performance execution was attempted. All changed source/test/configuration paths and identified risk questions have evidence-based dispositions; no unresolved review candidate or source-coverage gap remains.

Review converged after 3 round(s).

Reviewed exact head 0c4fa140e43bd3be71e097713f0cbd3f25d38c8c against the supplied aggregate diff and all 22 changed files. Two new verified P2 compatibility findings are inline: bare Lance home aliases and initial file-URI drive segments can select a different dataset or reject a valid in-root read. No new P0/P1 was verified.

Existing review context was independently revalidated: signed-zero/IPv6 P1 comment 4361447979 and percent/dot/whitespace P2 comments 4361447989, 4365938520 (including the bare-dot reply), and 4366084340 are fixed for their reported triggers. None is reposted; no existing P0/P1 remains applicable. No additional user-provided focus was supplied.

Critical checkpoints (static conclusions):

Static-review coverage includes all changed paths and the affected caller/downstream mechanisms. Runtime parser fuzzing, builds, test execution, full Groovy regression and actual remote-provider reads were outside this review's static-only contract. These limits are not claims of executed validation. Review convergence is assessed separately from the compatibility findings and delivery.

Review converged after 2 round(s).

Request changes: one new P1 finding. The new LOCAL disable switch still admits a native HDFS sink configured with Hadoop's local filesystem, allowing a global LOAD user to create BE-local files while enable_local_tvf=false. The inline finding includes the SQL trigger and verified planner-to-native-writer path.

Reviewed head 66d2b918a4cbcc4a5dca2271396700da91bbe365 against the supplied aggregate diff. All 23 changed files and affected read/write/schema/scan paths were covered. Review convergence is separate from correctness: the review converged after two rounds with this P1 outstanding.

The existing signed-zero/IPv6 P1 is fixed by original-port ASCII validation and IPv6 checks before metadata loading; no existing P0/P1 remains applicable. Existing percent-decoding, URI/bare dot-segment, whitespace and home-alias findings are addressed. The earlier exact triple-slash drive claim is contradicted by the pinned parser; related single-slash/rootless/authority-drive forms are now handled. These issues were independently rechecked and were not reposted. No additional user-provided focus was supplied.

Critical checkpoint conclusions:

1. Goal and proof: direct LOCAL/FILE reads and local/JNI writes are gated; native Hadoop local writes remain a gap. New tests exercise direct admission and path identity but omit this native sink case.
2. Scope and clarity: changes are focused on configuration, admission, range tagging and local path resolution, with matching test/config updates.
3. Concurrency: no new threads, locks or shared mutable query state; existing glob/schema workers and sequential reader ownership remain. No new lock-order or heavy-under-lock problem found.
4. Lifecycle and memory: resolver temporaries, the thread-safe passwd fallback and schema dataset ownership introduce no identified leak, ownership cycle or cross-TU initialization dependency; reader/session cleanup remains unchanged.
5. Configuration: enable_local_tvf is intentionally startup-only and defaults off; messages require every-FE restart. Seven pipeline configurations explicitly opt in.
6. Compatibility: default-off and privileged local-read admission are intentional behavior changes. Existing per-range tags and legacy scan-level fallback are compatible; pinned catalog snapshots retain their routing. LOCAL latest admission covers bare paths and file URIs; other SDK-local aliases were reviewed as outside that interface.
7. Protocol definitions: no Thrift/protobuf definition, field ID, enum or storage format changed.
8. Parallel paths: LOCAL/FILE, SELECT/EXPLAIN, INSERT/EXPLAIN, direct sink binding, JNI class aliases, ordinary glob and Lance schema/scan paths were traced; the verified native-Hadoop omission is the finding.
9. Conditional checks: FILE_LOCAL/version-zero selects LOCAL latest datasets; positive-version catalogs and remote ranges preserve their URI/options. Comments explain normalization order, legacy tags and JNI slash/NUL handling.
10. Test coverage: FE and BE assertions cover admission, authorization, backend routing, empty/NUL/remote/escape inputs and path-identity collisions with distinguishable schemas/rows. Native Hadoop local-provider rejection lacks coverage.
11. Expected results: no golden output file changed. Reviewed new schema/row/error assertions and cleanup/config restoration; no incorrect expected result identified.
12. Observability: admission errors identify configuration/privilege failures; containment errors hide expanded external names. Existing dataset error/profile context remains, with no added per-row logging.
13. Transactions/persistence: no visibility, delete-bitmap, EditLog/replay, checkpoint or master-failover protocol changed.
14. Writes and crashes: existing nontransactional TVF writer/cleanup behavior is unchanged; the new admission guarantee is incomplete as reported. No separate introduced atomicity or crash-lifecycle defect found.
15. FE/BE propagation: Lance ranges now carry the TVF's LOCAL/S3 identity; schema requests already do so. Both schema open and scan dataset-key creation use the same validated local resolver.
16. Performance and further risks: parsing/canonicalization runs at file/dataset setup, not row decoding. No substantiated additional performance or materialization defect found after the missed-issue sweep.

Validation limits: static review only; no builds, tests, runtime parser comparisons or deployment checks were executed. Author-reported test runs were treated as context, not reproduced results. Unchanged SDK external-manifest-base semantics were inspected and excluded as pre-existing. No unresolved PR candidate or assigned coverage gap remains.

Review converged after 2 round(s).

Static review of #69528 at fb92e7d found one new P2: guarded ViewFS listings retain distinct S3A child clients across paths without bounded cleanup. The inline comment describes the reachable configuration and ownership chain.

All 64 changed paths and their affected callers were covered. Existing P1 comments 4366428146 (native Hadoop writes), 4366880439 (Hadoop reads), 4366880442 (Lance manifest bases), and 4361447979 (signed-zero ports) were independently revalidated. Their original and recorded follow-up variants are addressed on this head; none was confirmed still applicable, so no existing blocking IDs are retained and no known finding is reposted.

Critical checkpoint conclusions:

Validation limits: static source review only. No builds, tests, runtime probes, benchmarks, submodule initialization or repository source changes were performed. Optional remote-service/io_uring controls and binary fixture behavior were not executed. These limits do not leave an unresolved source-review candidate.

Review converged after 2 round(s).

Static review of f293dce: one new P2 finding is included inline. A malformed file-URI authority can silently select a different LOCAL Lance dataset. No additional user focus was supplied.

Existing findings: independently revalidated P1 comments 4361447979 (signed-zero port), 4366428146 (native local sinks), 4366880439 (effective Hadoop read routes), and 4366880442 (manifest storage bases); their reported scenarios are addressed at this head, so existing_blocking_comment_ids is empty. The related P2 ViewFS child-lifetime concern in comment 4369693987 still has a native BrokerProperties-passthrough variant: per-prefix file_path values enter the frozen configuration profile while default-cached child clients can outlive the outer native handler. This is recorded as already covered, with no duplicate inline.

Critical-checkpoint conclusions:

• Goal: the default-off LOCAL gate, early read privileges, canonical confinement and effective Hadoop consumer checks implement the intended access boundary for the reviewed routes; the inline URI-identity regression needs correction.
• Scope and reuse: the changes are broad but connected to this boundary; the shared Java route interpreter is reused by FE projections and runtime checks, and existing factories/readers remain the IO owners.
• Concurrency: native handler lookup/insertion uses the manager mutex and exact guarded identities; connection creation and JNI path checks occur outside that mutex. The bthread-to-pthread bridge waits for completion; Java request-owned mount clients use synchronized ownership and idempotent close. No new lock-order defect was substantiated.
• Lifecycle/static initialization: request-owned Java ViewFS children and Avro streams/readers are cleaned up on partial opens and exceptional closes. Native global child caching has the already-covered residual above. No new cross-translation-unit initialization dependency was found.
• Configuration: enable_local_tvf defaults to false and is startup-only; every serving FE must restart after opt-in. Regression pipeline configurations explicitly opt in.
• Compatibility: TLanceFileDesc field 7 is optional is_tvf; HEAD and current master/4.2/4.1/4.0/3.1 definitions show no conflicting ID. Policy markers and the patched Hadoop/Lance ABI require matching serving components; mixed-version enforcement is not guaranteed, as disclosed by the PR.
• Parallel paths: checked LOCAL/FILE discovery and authorization, native INSERT/EXPLAIN and FE cleanup, HDFS and S3-to-Hadoop routes, declared-schema/provided-file shortcuts, Avro schema/data readers, and Lance schema/scanner/index/row-id paths. The existing local JNI writer is gated; unrelated writers with incompatible constructor interfaces cannot reach a local TVF write.
• Conditional semantics: selected ViewFS mounts, create-parent versus literal cleanup, scalar versus multi-target comma parsing, runtime defaults/provider overrides, exact authority identity, and URL versus literal filesystem paths were traced against consumers. The new inline addresses an uncovered SDK URL-parse-failure case.
• Functional/negative tests: inspected gate/auth, containment/symlink, path-identity, manifest-base, real-row, configuration/provider, cache-collision, glob, exceptional-close and paired-installation tests. They do not cover the malformed file-authority collision in the inline.
• Expected results: no changed regression .out files; the generator uses SDK APIs for the new binary manifests and the tests expect the existing fixture rows. This review did not regenerate fixtures or execute tests.
• Observability: rejection paths avoid exposing expanded external names; existing reader/IO profiles remain in use. No separate logging or metrics defect was substantiated.
• Transactions/persistence: no EditLog, transaction-version, delete-bitmap or internal persistence protocol changes; master-failover and internal committed-data visibility contracts are unaffected by these changes.
• Writes/crashes: reviewed admission before filesystem creation/cleanup and recoverable individual/pair SDK installation markers. External sink atomicity remains its existing contract; no new crash-recovery defect was substantiated.
• FE-to-BE propagation: native policies reach actual range/schema consumers, including HDFS parameters and Avro's separately created Java configuration; Lance TVF identity reaches schema and execution ranges, while unmarked positive-version catalogs keep their existing policy.
• Performance: direct remote cached native checks avoid repeated JNI thread dispatch; LOCAL Lance uses one whole-dataset split and tree preflight is linear in visited directories/files. No additional actionable performance issue beyond the known native client retention was verified.
• Other issues and limits: every changed path and initial risk was reviewed and dispositioned. This was static-only: no builds, tests, live storage IO or submodule initialization. Local tree preflight is not an atomic filesystem snapshot; optional io_uring/remote-service execution and mixed-version runtime behavior were not validated here.

All new verified findings are included in the inline set; previously reported concerns are handled as described above.

Review converged after 2 round(s).

Static review of head 95b42fa. One new P2 compatibility regression is reported inline: finite remote ViewFS aliases can be rejected as cycles. No additional user focus was supplied.

Critical checkpoints:

• Goal and correctness: default-off LOCAL admission, early ADMIN/OPERATOR read checks, canonical containment, effective Hadoop runtime routing and Lance manifest-base checks are implemented. The inline finding identifies a valid remote route rejected by the new guard.
• Scope and clarity: the changes span FE, BE, the pinned SDK and build integration because all participate in storage admission; no unrelated correctness issue was verified.
• Concurrency: cache lookups/insertion/removal are locked; native creation and route checks run outside that mutex. Java request-child ownership is synchronized and isolated between requests; no new lock-order defect was verified.
• Lifecycle and initialization: checked dataset RAII, failed-open cleanup, Avro reader-before-filesystem close, owned ViewFS peers, JNI global-reference release on pthreads and configuration freezing after provider resource registration. No new static initialization dependency was identified.
• Configuration: enable_local_tvf is intentionally startup-only, false by default, with restart instructions; regression profiles explicitly opt in.
• Compatibility: the C metadata APIs are additive. TLanceFileDesc field 7 is optional and does not collide in the inspected master/4.2/4.1/4.0/3.1 definitions. Full enforcement requires upgraded serving FE/BE and the matching patched Hadoop/Lance artifacts; old peers cannot enforce the new marker.
• Parallel paths: reviewed LOCAL/FILE/S3/HDFS, listing and returned paths, schema and data, native and Avro, writes and cleanup, cached handles and unmarked catalog controls.
• Special conditions: checked URI/C-string projections, provider selection, ViewFS mounts/globs, aliases and SDK fallback identity. The reported table-only recursion condition is too broad; local targets and real cycles still need rejection.
• Test coverage: inspected positive/negative admission, parser collision, real Lance row/schema, manifest-base, cache isolation, lifecycle, projection/split and interrupted-installation test source. Builds and tests were not run under this static-only contract; remote-service and opt-in io_uring execution are unverified.
• Test results: no expected .out files were changed; no runtime pass or full-CI claim is made.
• Observability: existing IO profiles remain; new admission failures explain the required route/opt-in while containment errors avoid exposing expanded external paths. No new per-row logging was introduced.
• Persistence and failover: no new EditLog, transaction, visible-version or delete-bitmap state is introduced.
• Writes and atomicity: admission precedes the affected filesystem side effects; external sink transactionality and cleanup semantics remain those of the existing implementation. No new atomic-commit mechanism is claimed.
• FE/BE propagation: checked native properties/HDFS schema parameters, Avro JNI markers, Lance range type/TVF marker, guarded cache identities and removal of user-supplied internal-policy aliases.
• Performance and memory: checked configuration-profile reuse, exact connection/cache isolation, request-child cleanup and recoverable paired SDK publication. LOCAL Lance uses one whole-dataset split; no per-fragment tree-traversal regression was established. No additional verified hot-row or allocation defect was found.
• Additional risk sweep: the HDFS cross-filesystem symlink hypothesis is unreachable with the reviewed production-global symlink gate disabled; SDK parser/base identity and cached-context paths were rechecked without another verified finding.

Existing P1 comments 4361447979 (signed ports), 4366428146 (native sinks), 4366880439 (Hadoop reads) and 4366880442 (manifest bases) were independently revalidated against this head and their described defects are addressed. They are not reposted and no existing P0/P1 remains independently confirmed as applicable.

Converged in two rounds after complete assigned static coverage and an unchanged-payload final recheck. No unresolved candidate or static coverage gap remains; runtime validation and mixed-version enforcement remain the limits stated above.

Review converged after 2 round(s).

Static review of PR #69528 at b1fb0a3. One new P2 finding: unchanged remote Lance snapshots repeat full storage-base inspection for every fragment split, creating multiplicative setup work for many-base datasets. See the inline comment.

Critical checkpoints:

• Goal and proof: default-off LOCAL admission, authorization before discovery, canonical containment, and actual Hadoop/Lance consumer checks are present in the inspected read/write flows. Focused positive/negative tests are included; this invocation performed source inspection only.
• Scope: changes stay within TVF admission, storage/path/cache identity, supporting SDK/build controls, and tests. Shared routing logic is used by FE and JVM consumers.
• Concurrency and locks: guarded HDFS cache identity/selection and cleanup use the manager mutex and shared ownership; expensive creation and JNI checks occur outside that lock. Request-owned ViewFS children synchronize creation/close. No additional lock-order defect was verified.
• Lifecycle and initialization: SDK strings/temporary handles use RAII; failed validation does not publish a dataset. Avro closes partial opens, and ViewFS owns its child clients. No new cross-translation-unit static initialization dependency was found.
• Configuration: enable_local_tvf defaults to false and is startup-only; the description correctly requires configuration and restart on every serving FE.
• Compatibility: supported URI/provider identities and unmarked catalog routing were traced. Full enforcement requires upgraded serving FE/BE and matching patched runtimes; old BEs ignore the new policy marker.
• Shared definitions: the optional TLanceFileDesc field 7 was compared with available branch-3.1/4.0/4.1/4.2/master definitions; no new collision, reused ID, or required field was found.
• Parallel paths: LOCAL/FILE, S3/FILE, HDFS/FILE, INSERT/EXPLAIN/direct binding, FE cleanup, schema RPCs, native/Avro data reads, and fresh/cached Lance handles were checked.
• Special conditions: native string projection, create-parent routing, literal cleanup versus glob discovery, ViewFS aliases/cycles/depth, schema-skipping CSV, and SDK local fallback rules have explicit handling and focused tests.
• Test coverage: changed FE/BE/Avro/SDK/build-control/regression tests were inspected, including identity collisions, inherited providers, manifest bases, ownership failures, and interrupted installation. Remote-service and io_uring controls remain opt-in.
• Results: no new expected-output files were added; expected error paths were inspected. No builds, tests, storage data IO, or submodule initialization were performed by this review. Binary fixture base references/footer were checked statically; row semantics were not executed.
• Observability: admission failures return explicit errors and containment messages avoid expanded outside names. Existing IO/profile instrumentation is retained.
• Persistence and transactions: Doris visible-version, EditLog, transaction, and MoW protocols are not modified.
• Writes and crash behavior: admission precedes consumer IO; existing file-write/delete atomicity semantics are retained. SDK header/archive and paired Rust repair use persistent incomplete markers.
• FE/BE propagation: is_tvf is set for schema and execution ranges; native read/write policy reaches actual consumers, including Avro's independent configuration.
• Performance: the inline P2 covers repeated immutable remote-base reconstruction. The initially suspected LOCAL per-fragment tree-walk issue was dismissed because LOCAL uses one whole-dataset split.
• Other concerns and user focus: no additional user focus was supplied. The HDFS symlink hypothesis was dismissed because the required global symlink-enablement path is absent in the reviewed production sources.

Existing P1 threads 4361447979 (signed-zero URI fallback), 4366428146 (native Hadoop sinks), 4366880439 (Hadoop reads), and 4366880442 (Lance manifest-selected bases) were independently revalidated against this head. Their reported scenarios are addressed; no existing blocking ID is carried and no duplicate comment is posted.

Known limits: cross-mount Hadoop globs retain the explicitly disclosed requirement for a proven route such as a remote fallback; local tree preflight is not an atomic filesystem snapshot; mixed-version enforcement and runtime/performance behavior were not executed here. No unresolved source-review candidate or assigned coverage gap remains.

Review converged after 2 round(s).