suxiaogang223 commented on PR #70112: URL: https://github.com/apache/doris/pull/70112#issuecomment-6653944060
Local code review completed for **6fdeaefa58e8d8193e543e12936d3613dfeb0536**, against `master` base **856194ece0e1e0ad45b46b96f26a3e2859412e06**. Reviewed the complete 27-file diff with the local `code-review` skill, the applicable FE/connector/BE guides, and the upstream/downstream scan and write paths. **Result: PASS. No actionable code findings in the reviewed diff.** Validation limitations are recorded below; this verdict does not represent an all-checks-passed assertion. The read behavior follows [Iceberg's format specification](https://iceberg.apache.org/spec/#parquet): a corresponding physical column is ignored when the bound read schema declares `unknown`. The review traced this flow: ```text Iceberg UNKNOWN -> connector UNKNOWN -> ConnectorUnknownType / NULL_TYPE -> scan schema with field IDs -> TableColumnMapper constant/missing child mapping -> TableReader NULL materialization, preserving container shape ``` Critical checkpoint conclusions: | Checkpoint | Conclusion | | --- | --- | | Goal and proof | UNKNOWN becomes queryable as NULL while target tables containing UNKNOWN remain read-only. Tests cover metadata display, NULL execution, nested reads, schema promotion, historical snapshots, and write rejection. | | Scope and reuse | The implementation reuses the SQL NULL carrier, mapper constants, default materialization, and existing write-validation boundaries. Other unsupported Iceberg types and connector SPI surfaces are unchanged. | | Concurrency and locking | No new thread, shared mutable cache, or lock-order change. Schema traversal is local to a request. Existing begin-write locking, metadata-generation checks, and final schema/identity validation remain in place. | | Lifecycle and memory | New expressions use the existing expression-context ownership/open/close path. Column construction uses existing datatype/column allocation and COW paths. No new reader owner, resource lifetime, or cross-TU static initialization dependency. | | Configuration | No new FE/BE configuration or session variable. | | Compatibility | No on-disk layout change. The logical type has a Gson subtype registration and round-trip coverage. FE/BE must be deployed with the accompanying semantics; mixed-version execution is explicitly unverified. | | Thrift/Protobuf | No definition, field ID, or enum value is added or changed; the existing NULL_TYPE is used. | | Parallel paths | Mapping works by ID/name/index and nested reconstruction is shared by Parquet/ORC. All predicate conversion modes use the UNKNOWN exclusion. Write guards cover writer binding, transaction initialization, metadata DELETE, and rewrite planning. | | Conditions and invariants | UNKNOWN is synthesized before physical/partition matching. Nested parents retain null maps, offsets, and Map keys. Full Map shape is retained where the file reader requires both streams. Current UNKNOWN equality keys use typed NULL; dropped historical keys retain concrete physical-type resolution. | | Coverage | FE tests exercise type/Gson round trips, pinned rebinding, nested field IDs, pushdown exclusion, and write guards. Four BE tests cover mapping, nested shape, and NULL/non-NULL equality tombstones. Runtime regression exercises absent/present physical columns, empty tables, multiple batches, evolution, time travel, and negative writes. | | Expected results | The 219-line golden was generated by the regression runner and passed comparison mode. All 44 result blocks were inspected; all 22 Parquet/ORC pairs match. The suite checks 29 expected errors. | | Observability | Unsupported writes return an explicit diagnostic through existing connector exception handling. Existing scan profiles and error propagation are retained; no new execution subsystem requires separate metrics. | | Persistence and failover | Gson retains UNKNOWN identity across serialization. No EditLog format or transaction persistence protocol changes. Bound schemas remain separate objects; rebinding a promoted type does not mutate an existing read. | | Writes and atomicity | The authoritative target schema is checked recursively, including omitted columns. Checks run before metadata-only DELETE or empty rewrite shortcuts and again around transaction refresh. Existing commit/abort and schema-change fences remain in use. | | FE/BE values | The NULL type is carried in both regular and equality-delete scan schemas; field IDs and nested nullability are preserved. No new separately transmitted variable. | | Performance | Top-level UNKNOWN avoids decoding an ignored value. NULL synthesis reuses batch materialization. Schema scans are outside row loops. A Map UNKNOWN value may retain its physical value stream for the existing reader's structural contract; no new performance claim is made. | | Other concerns | No new actionable defect found. Mixed-version execution and the pending user-documentation update remain disclosed limitations. Local BE unit execution and clang-tidy completion are not claimed. | Validation evidence in this worktree for the reviewed implementation: - Previously completed FE and ASAN BE builds using `build.sh` and the downloaded prebuilt dependencies; no third-party rebuild. - `run-fe-ut.sh`: **150 targeted FE tests passed** (114 Iceberg connector tests and 36 converter tests), no failures or skips. - Regression framework: **50 unit tests passed**. - `run-regression-test.sh -d external_table_p0/iceberg -s test_iceberg_unknown_read`: output generation and subsequent golden comparison both passed, with no failed/skipped suite. Fixtures were compiled/run through the Spark-container test entry point. - Rechecked during this review: BE build/header hygiene and connector import gates passed; the 44 golden blocks and 22 format pairs matched. - The current head also reports successful CI [BE UT, build 1092607](http://43.132.222.7:8111/viewLog.html?buildId=1092607&buildTypeId=Doris_DorisBeUt_BeUt) and [External Regression, build 1092628](http://43.132.222.7:8111/viewLog.html?buildId=1092628&buildTypeId=Doris_External_Regression). These are CI results, not local BE unit-test results. Validation gap: the explicit full-diff clang-tidy invocation could not complete because the available PCH was built for branch-4.2 and became invalid after switching back to this master-based PR. Diagnostics produced with that stale PCH are not represented as a successful analysis. The earlier no-changed-files clang-tidy invocation is likewise not counted as coverage. <!-- doris-repo-review:v1:begin --> ```yaml schema: doris-repo-review/v1 status: PASS pr: apache/doris#70112 commit: 6fdeaefa58e8d8193e543e12936d3613dfeb0536 base: 856194ece0e1e0ad45b46b96f26a3e2859412e06 reviewed_at: 2026-11-05T09:00:09+00:00 reviewer: suxiaogang223 model: gpt-6.1-sol effort: xhigh findings: {blocker: 0, major: 0, minor: 0, nit: 0} rounds: 1 converged: true ``` <!-- doris-repo-review:v1:end --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
