Gabriel39 commented on PR #65851: URL: https://github.com/apache/doris/pull/65851#issuecomment-5118663606
Current-head review summary (`de3a2d51fc8`) The four issues from the previous review round have substantive fixes, but the current head is still not merge-ready. I verified the following unresolved findings against the latest code: 1. **P1 — Bind filtered data manifests with the current schema.** The two-argument `ManifestFiles.read` overload can bind a current-name predicate against a historical manifest schema and fail after a rename: https://github.com/apache/doris/pull/65851#discussion_r3673466007 2. **P1 — Preserve nullable collection masks during ORC normalization.** ARRAY/MAP recursion drops the parent skipped-row mask, so invalid hidden UUID/FIXED elements under a NULL collection can reject a semantically valid write: https://github.com/apache/doris/pull/65851#discussion_r3673466013 3. **P1 — Fence writes to the pinned Iceberg table identity.** Schema/spec/order IDs do not distinguish a dropped and recreated table; the context must pin and validate the Iceberg table UUID at preflight and final commit replay: https://github.com/apache/doris/pull/65851#discussion_r3673466022 4. **P1 — Reject optional branch fields that are required in the current schema.** A physically present NULL is not replaced by `initialDefault`, so every present optional-to-required mismatch must be rejected regardless of the default: https://github.com/apache/doris/pull/65851#discussion_r3673780883 5. **P1 — Resolve the full name path for ID-less nested equality keys in V1.** The current V1 name fallback searches only top-level columns and can hash a default/NULL instead of the stored nested value, diverging from V2: https://github.com/apache/doris/pull/65851#discussion_r3673780890 6. **P1 — Align equality preflight with manifest-cache dispatch.** The preflight can row-filter away a delete that the cached task path later attaches, leaving its field ID out of the schema carrier and compatibility fence: https://github.com/apache/doris/pull/65851#discussion_r3673780894 7. **P2 — Avoid a synchronous duplicate manifest pass in batch mode.** With any retained equality delete, the FE preflight still reads matching delete and data manifests before asynchronous dispatch and the real planner reads them again: https://github.com/apache/doris/pull/65851#discussion_r3674888290 There are also two hard integration gates: - GitHub currently reports `mergeable=false` and `mergeable_state=dirty`. The PR conflicts with the target branch in `IcebergMergeSink.java`, `LogicalIcebergMergeSink.java`, `PhysicalIcebergMergeSink.java`, and `PhysicalPlanTranslator.java`. - The current FE UT, BE UT, Cloud UT, Compile, and Performance TeamCity builds all finished with failure because the PR could not be merged with the target branch during setup. Those pipelines did not reach their actual compile/test workloads, so the latest head does not yet have valid merged-base Linux FE/BE/Cloud/compile coverage. Check status: https://github.com/apache/doris/pull/65851/checks Please resolve the six correctness/interoperability findings, address or measure the batch-planning regression, rebase and resolve the four conflicts, and rerun the full TeamCity set before the next merge-readiness review. -- 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]
