hubgeter commented on PR #65851: URL: https://github.com/apache/doris/pull/65851#issuecomment-5179040664
> Request changes on [b3fd815](https://github.com/apache/doris/commit/b3fd8150f2caea8fe809f6e6d06a36dad88be495). > > This update correctly addresses two of the three findings from the previous head: the V1 Parquet/ORC equality-key lookup now reuses the ordinary mapper's unique ID-less-wrapper rule and preserves the physical nullable parent, and the connector SPI change is now explicitly versioned as API 4.0 with the newly returned provider/handle surfaces frozen. The snapshot-history fix is incomplete: it skips traversal only when the schema set proves that no historical state is relevant. Once one relevant older schema exists, both the required-field gate and the equality carrier still walk every intervening same-schema snapshot, so planning remains O(retained snapshots). I added the concrete two-schema/long-tail case as a reply to the existing P2 thread rather than opening a duplicate. > > The PR is also not mergeable while the existing unresolved P1 threads remain, including current-head correctness/crash cases around nullable ORC collections, prepared-statement write-schema state, omitted nullable MERGE columns, V2 typed/default carriers, backslash defaults, missing complex-root materialization, projection-aware/plugin upgrade fencing, applicable-delete gating, and the overwrite pin boundary. Those are existing live findings and are intentionally not repeated as new inline comments here. > > Critical checkpoint conclusions: > > * Goal and proof: The end-to-end Iceberg v3 default/evolution goal is still not complete because existing live P1 paths can return wrong rows/values or abort a BE. The new V1 wrapper regression is directionally correct, but the new history counter test proves only the trivial one-schema fast path. > * Scope and clarity: The 10-file incremental change is focused on the three latest findings. The wrapper and API-version changes are small and reuse existing contracts; the history refactor claims a stronger bound than it implements. > * Concurrency and lifecycle: This increment adds no new thread, lock, or lifetime boundary. Existing statement-scope lifecycle concerns remain represented by live threads; no additional distinct race or lock-order issue was found here. > * Configuration and compatibility: No new runtime configuration is added. Bumping connector.plugin.api.version to 4.0 and freezing ConnectorColumnHandle/ConnectorWritePlanProvider closes the reviewed plugin compatibility gap. > * Parallel paths and conditions: Parquet and ORC use the same unique-wrapper semantics and the test covers both. The history review covered ordinary required-field gating and the equality-delete carrier; both retain the long same-schema-tail cost. > * Tests and results: The update adds focused BE/FE tests, but its commit message explicitly says the unit tests were added without being compiled or run. Current CI is still pending. I performed a static review only and do not claim runtime validation. > * Error handling, observability, persistence, and transactions: No new status-swallowing, logging/metrics, Doris EditLog, or commit-atomicity issue was introduced by this increment. Existing write-path blockers remain in their live threads. > * FE/BE propagation and memory safety: The wrapper fix aligns V1 physical-path discovery with the shared mapper and preserves nullable-parent semantics. No new memory-safety defect was substantiated in this increment. > * Performance: The remaining P2 is concrete: S1 marks a projected field optional, S2 requires it, and 100k later S2 snapshots force 100k parent lookups on every relevant query before S1 is reached. A dropped historical equality field produces the analogous carrier path. > > Recommendation: do not merge. Resolve the outstanding P1 threads, finish the history bound with a two-schema long-tail oracle, and run the focused unit/regression suites before reconsideration. I agree that the snapshot-history optimization is still incomplete. I plan to address the snapshot-history optimization separately, together with dedicated two-schema and long-tail tests -- 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]
