Review status

COMPLETE static review of PR #68853 at head 04bc39642665a04901cbacae6dbf8420c11e0014 against base 8d0eacd574c37c45f8bc8ad99f053e9ec8b564bb. I reviewed the authoritative 227-path diff, relevant upstream/downstream paths, added tests and fixtures, existing threads, and three convergence rounds. Five distinct new inline findings remain: one P1 JSON load row-integrity issue and four P2 S3/optimizer issues. There are no existing inline threads or user focus points to carry forward. All suspicious candidates have been accepted, deduplicated, or dismissed; convergence is complete.

Critical checkpoints

• Goal and proof: The six-field FILE type is wired through FE/BE planning, casts/functions, wire formats, ORC/Arrow output, JSON input, and S3 TO_FILE/LIST_FILE. The added tests provide static oracles for many valid and invalid values, but the five inline cases show the feature is not fully correct. Builds and tests were prohibited by this review contract, so none were run.
• Scope and reuse: The broad changes are feature-related and generally reuse struct/nullable, SerDe, TVF, and resource patterns. I found no unrelated change to call out.
• Concurrency and locks: TO_FILE pins the selected filesystem while a mutex protects bucket switching; remote HEAD runs outside that lock. Query resource snapshots, LIST_FILE pagination/cancellation, and column copy-on-write were traced; no separate race, lock-order, or deadlock issue was substantiated.
• Lifecycle and memory: Query-owned resources and filesystem pointers have bounded lifetimes; six-child ColumnFile/VARBINARY arena ownership remains aligned across filter, cache, spill, and transport paths. I found no shared-pointer cycle, orphan allocation, or cross-translation-unit static initialization dependency.
• Configuration: FILE transport is gated by BE execution version 17. No new dynamic configuration item requires propagation; the new recursive TVF option has an Azure-provider semantics defect (inline finding).
• Compatibility and storage: Touched Thrift/Protobuf values and field IDs were compared with branch-3.1, 4.0, 4.1, 4.2, and master; no reused ID or incompatible existing-struct field was found. FE/BE six-child FILE encoders/decoders and old-version gates agree in the inspected paths. ORC sparse-child and nested-null compatibility were checked in both reader paths.
• Parallel paths, conditions, and error handling: v1/v2 JSON, both ORC readers, Arrow, CSV, Parquet, S3 listing, and HEAD were traced. Strict v1 JSON FILE failure does not roll back an earlier field from the row (P1); the other inspected malformed-value paths return errors or preserve alignment. LIST_FILE raw-key and Azure depth conditions have separate P2 findings.
• Tests and results: Added BE/FE unit tests, regression suites, ORC fixtures, and expected outputs were inspected, not executed. The strict top-level invalid FILE, special S3 key round trip, Azure nonrecursive depth, non-null FILE literal cost, and negated FILE null-stat cases lack effective coverage.
• Observability: Existing status/error counters and logs cover direct FILE validation and S3 failures; I found no separate logging or metric defect. The JSON row-integrity path needs an atomic rejection as described inline.
• Transactions, persistence, and writes: FE catalog/Gson/Column persistence was checked; no EditLog, visible-version, delete-bitmap, or transaction protocol change is introduced. JSON load row atomicity is broken by the P1 finding; other inspected FILE writes validate before publishing output.
• FE/BE variables: Checked S3 resources flow through coordinator, alternate fragment builder, and direct load planner into BE query context; no missing send path was established.
• Performance and remaining issues: Non-null FILE literal size/numeric statistics and negated FILE null Boolean statistics produce the two P2 costing findings. No other demonstrated hot-path regression or unresolved candidate remains.

Review execution on 326d74e175bfadcae63285bf4226c291214e09a6 is incomplete for convergence: a new valuable finding appeared in the third and final permitted review round. The 228 changed paths and all candidate points were swept, and the four confirmed new P2 issues are inline. This was a static review only; no builds or tests were run.

Critical checkpoints

• Goal and scope: The PR adds a six-field FILE value through FE/BE expressions, transport, external ORC, TO_FILE, and LIST_FILE. The changes are broad but feature related. The implementation does not fully deliver consistent URI and metadata behavior because of the four inline issues.
• Concurrency and lifecycle: Query-scoped S3 resource snapshots are immutable; TO_FILE shares fragment state across clones, locks filesystem switching, and performs remote HEAD outside the lock. Ownership and close paths showed no additional supported race, deadlock, or retention issue.
• Configuration and compatibility: The fixed FILE execution-version gate is 17. New thrift fields are optional; touched thrift/protobuf IDs were compared with release branches, and FE/BE format gates were checked. No separate rolling-upgrade or dynamic-configuration defect was substantiated.
• Parallel paths and conditions: Both ORC readers, sparse/nullable projections, JSON and block transport, CSV/Parquet/Arrow boundaries, S3 and Azure clients, and legacy/New Planner/stream-load resource senders were traced. The supported gaps are the four URI/metadata cases in the inline comments.
• Tests and results: Changed BE and FE unit tests, regression suites, and expected outputs cover principal FILE operations, error handling, and transport. They lack the raw-key MIME, Azure HEAD/list ETag, and HTTP(S) LIST_FILE cases reported here. Their runtime results were not verified because execution was prohibited.
• Observability, persistence, and writes: Remote failures and cancellation have status paths; no separate observability issue was supported. Internal FILE columns are gated; FE persistence/replay and external ORC output were inspected. No distinct transaction, crash-atomicity, or write-path defect was substantiated.
• FE/BE variables and performance: The S3 resource map is sent on the reviewed coordinator, distributed-planner, and stream-load paths. Remote HEAD remains per row, while client switching is lock scoped and LIST_FILE is paged; no separate actionable performance issue was substantiated.
• Other findings and existing threads: The two existing P2 statistics issues (non-null FILE literal size and IS NOT NULL range) still apply and are not reposted. The existing raw-key lookup and Azure nonrecursive LIST_FILE P2s are fixed. The existing JSON P1 describes an OK/valid=false continuation that no longer occurs: strict FILE deserialization errors propagate through RETURN_IF_ERROR, so its ID is not carried as a blocking comment. There was no additional user-provided focus.

The three-round cap ended with the Azure LIST_FILE ETag finding in round three, so this review does not claim full convergence. All four supported issues found within the permitted rounds are included below.

Review complete for 3647cb947a5e7c465fdfb664467300f7299edeee (static only). No new inline findings. The existing P2 threads on FILE literal and IS NOT NULL statistics remain applicable and are not duplicated. The earlier P1 JSON row rollback thread does not apply to this head: the FILE-specific success-with-invalid-row branch is gone, and invalid FILE decoding now propagates an error rather than continuing with a partially appended row. No existing P0/P1 blocker remains applicable.

Critical checkpoints:

• Goal and proof: The change adds a six-field FILE value through FE/BE types, wire transport, TO_FILE, LIST_FILE, casts, field access, and EXPLODE_FILE. The added BE/FE unit cases and regression suites exercise those paths, including invalid values and resource access. Their execution was prohibited for this review, so this is a static conclusion.
• Scope and clarity: The 185 changed paths stay within the FILE feature, supporting object metadata and version transport, and their tests. No unrelated change was identified.
• Concurrency: TO_FILE protects lazy filesystem creation with a mutex and performs remote HEAD outside that lock. Query resources are collected per execution; LIST_FILE uses one generator task and streamed pages. No unprotected shared mutation or lock-order issue was substantiated.
• Lifecycle and initialization: Per-statement resource snapshots reach the first BE query context through every fragment construction path; TO_FILE pins the filesystem across HEAD and checks cancellation. No new cross-translation-unit static initialization dependency, ownership cycle, or unreleased handle was found.
• Configuration: The FE max BE execution version advances to the immutable FILE minimum, version 17. No dynamic configuration behavior is introduced.
• Compatibility and wire IDs: The touched Thrift/protobuf definitions were compared with release branches 3.1, 4.0, 4.1 and 4.2; no reused field or enum ID or new required field was found. FILE transport, constant values, cache/spill serialization, and version gating were traced for rolling upgrades.
• Parallel paths and conditions: Legacy coordinator, Nereids, and stream-load fragment paths all forward S3 resources. FE DDL and SQL key/function gates, nullable/constant casts, nested pruning, BE format readers, OUTFILE restrictions, and S3/Azure listing paths were checked; no distinct missed branch was substantiated.
• Test coverage and results: Inspected positive, negative, nullable, raw-key, pagination, authorization, cache, spill, format, and persistence cases plus their expected-output files. The test outcomes and generated files were not runtime-validated in this review; no build or test was run.
• Observability: New remote and conversion failures propagate contextual Status errors; existing query diagnostics are sufficient for the reviewed paths. No missing critical metric or excessive new INFO logging was identified.
• Persistence, transactions, and writes: FE FILE type/literal persistence registration and replay were reviewed. FILE storage and unsupported write/format routes are gated; this PR adds no transactional FILE write path or EditLog mutation.
• FE/BE state and performance: The resource map and FILE expression/type descriptors reach all reviewed FE/BE send paths. Listing is paginated, advances empty pages, and honors LIMIT before another fetch; no concrete CPU or memory regression was found.
• Other issues and focus: The supplied focus file contained no extra focus. All five review scans returned NO_NEW_VALUABLE_FINDINGS; the one apparent nested-cast pruning concern was refuted by recursive type-change checking. Final changed-path and candidate sweep is complete with no unresolved point.

Review status

Completed static review of PR #68853 at head afcc2850b3f7ab7a21412d1b9193ecca93d501ea against base 380912cc7344e92375c3de58ec37c99602cad013. No new inline finding is reported. The full changed-file set, authoritative diff, existing review threads, five risk focuses, and delegated review sections were rechecked. The review converged in round 1.

Critical checkpoints

• Scope and goal: The FILE feature was traced across the six-child BE column/value/SerDe implementation, FE and Nereids types, TO_FILE and FL_GET_BINARY, LIST_FILES/EXPLODE_FILE, resource propagation, object-storage metadata, query cache/spill, and FE/BE wire descriptors. The changes remain feature-scoped.
• Correctness: Parent and child null handling, filtering, constants, validation, JSON/PB transport, aggregate state, cache/spill paths, and result serialization were inspected. The regular JSON partial-object behavior is intentional and covered by FormatInputMatchesNamesAndFillsMissingOptionalFields; the JSONB six-field requirement is a separate strict cast contract.
• Analysis and operator guards: FILE casts, element access, grouping/order/set/window/distinct restrictions, nested pruning, Arrow/OUTFILE boundaries, and comparison coercion were checked. The proposed join/filter comparison concern is dismissed because supportCompare(..., false) rejects FILE before legacy translation.
• Resources and lifecycle: TO_FILE resource snapshots, prepared/routine/broker handoffs, filesystem bucket switching, cancellation, LIST_FILES pagination, output slot mapping, and Azure/S3 provider paths were reviewed. Existing resource, raw-key, MIME, Azure metadata, and aggregate-state P2 threads cover the remaining known issues; no distinct new race or lifecycle defect was found.
• Compatibility: Thrift/protobuf ids, FILE six-child descriptors, optional fragment/list fields, FE/BE factories, and release snapshots (3.1, 4.0, 4.1, 4.2, master) were compared. No reused id, changed meaning, or new blocking protocol incompatibility was substantiated.
• Existing blockers: Existing P1 4242705286 targets the removed legacy be/src/format/json/new_json_reader.cpp path, and existing P1 4282867982 is fixed by the current TYPE_FILE VCase dispatcher arm. Neither remains applicable, so no blocking ids are carried forward.
• Tests and validation: Added BE/FE unit tests, regression suites, expected outputs, and fixtures were inspected statically. Builds, tests, benchmarks, formatters, and runtime validation were not run under this review contract.
• User focus: review_focus.txt contains no additional focus. No unresolved candidate or coverage gap remains after the final changed-file and risk sweep.

Review converged after 1 round(s).

Review of head 003ed3a: two new verified findings, P1 FILE descriptor incompatibility during upgrades and P2 missing S3 resources on first stream/routine-load execution. The P1 needs correction before merge. Findings are supported by static producer/consumer traces; no builds, tests, benchmarks, or runtime provider/upgrade checks were executed.

The affected load flow is an ordinary scalar projection, so the FILE table-column prohibition does not prevent it:

OlapTableSink(file_size BIGINT)
  LoadProject(file_size := ELEMENT_AT(TO_FILE('res', uri), 'size'))
    LoadScan(uri STRING)

Critical checkpoint conclusions:

1. Goal and behavior: independent six-field query FILE values, metadata producers, JSON results, and nested value transport are integrated; the two inline findings identify incomplete load and upgrade behavior. Current explicit FILE casts are deliberately rejected.
2. Scope and reuse: the cross-module scope follows the type integration; fixed-schema helpers, shared field projection, and shared metadata/ETag conversion provide reuse. No further scope defect was established.
3. Concurrency and locks: resource copying uses the resource read lock; shared TO_FILE client/bucket changes use a mutex, with each remote stat outside it while retaining a shared filesystem. No additional race or lock inversion was found.
4. Lifecycle and static initialization: query-map alias ownership, RuntimeState/FunctionContext clone lifetime, column COW, and explode input cleanup were traced. New constant tables do not introduce a cross-TU initialization dependency.
5. Configuration: no new operator configuration item; normal SQL captures current resource properties during translation. Lazy client creation preserves NULL-URI behavior.
6. Incompatible changes: P1 changes already-supported branch-4.1 FILE child descriptors from VARCHAR to STRING without compatible readers/writers or a capability gate; both upgrade orders fail.
7. Thrift/protobuf allocation: all touched definitions were compared with branches 3.1, 4.0, 4.1, and 4.2. New numbers are unused or identical and existing structs gain optional members; matching ids do not cure the P1 child-schema mismatch.
8. Parallel paths: normal SQL, point-query fallback, prepared execution, S3/Azure, direct/recursive listing, and load entry points were checked. P2 is confined to the new first-execution stream/routine omission; the existing broker handoff issue is suppressed.
9. Conditional checks and errors: canonical child schemas, constant selectors, locator/range/metadata validation, cancellation, and output-format rejection have concrete failure paths. Deliberate unsupported direct-aggregate/Python/Arrow boundaries are not new findings.
10. Test coverage: inspected all changed BE/FE unit tests and regression suites, including null/empty/binary, malformed input, nested/constant/COW, metadata, pagination/LIMIT, cancellation, and concurrency cases. The load-map and mixed-version descriptor cases need coverage with their fixes.
11. Expected results: inspected changed outputs against suite SQL and ordering/count expectations; no additional incorrect result was established. Test execution and output-generation provenance were not verified.
12. Observability: metadata requests retain existing latency/rate-limit instrumentation and provider/path errors; no additional observability defect was established.
13. Persistence and transactions: both Gson registries include FILE. This change adds no transaction journal transition; table FILE storage is prohibited. No new visible-version/delete-bitmap mechanism is involved.
14. Writes and atomicity: new remote operations read metadata; expression errors avoid publishing partial results. Existing scalar transactional loads are affected by the P2 setup failure.
15. FE/BE variable passing: normal SQL carries resources through pipeline parameters to QueryContext; direct stream/routine planning drops its collector-local map. This is the P2 finding.
16. Performance: listing retains one provider page and the current batch; metadata getters share selected storage. Previously reported getter/constant-concat copy concerns are suppressed.
17. Further issues and coverage limits: all 158 changed paths and initial risk items received complete-review and integration coverage; candidates were accepted, dismissed with evidence, or deduplicated. No unresolved static-review question remains after the final round. Runtime correctness, mixed-version execution, and test success remain unexecuted validation limits.

Existing context: all 60 current inline comments and replies were checked. The old CASE P1 is fixed by the TYPE_FILE dispatcher arm; the old strict FILE JSON-input P1 path is absent and native FILE table input is disabled. Neither is independently confirmed as an existing blocker. Known P2 issues, including broker resource handoff, escaped aggregate-state metadata and mixed-inline copying, are not reposted. The new load finding requires neither credential rotation nor cached-plan reuse; the new compatibility finding concerns actual descriptor interpretation, beyond the earlier VARCHAR/STRING design question. No additional user focus was supplied.

Review converged after 3 round(s).