zeroshade commented on PR #1974: URL: https://github.com/apache/iceberg-go/pull/1974#issuecomment-5488575154
Update: I rebased #1965 and #1976 onto current `main` and merged both. I stopped short on this one, because its rebase is not mechanical and I did not want to guess at your design. The `schema.go` hunk drops out cleanly (already upstream). The real conflict is that `PlanFiles` has changed underneath this PR in three ways that interact with the streaming restructure: 1. **#1939 splits one entry into N tasks.** `splitParquetScanTask(task, splitTargetSize)` can emit multiple tasks per manifest entry. `planDataManifestTasks` sizes its `directBuffer` via `manifestTaskCapacity` at exactly one task per live entry, so that invariant no longer holds. 2. **#1971 computes per-task residuals.** Main keeps `residualEvaluators` as a plain `map[int]*partitionResidualEvaluator` populated per entry, which is fine sequentially but would race inside your `g.Go` workers. 3. **Result metrics accumulate per task.** `acc.resultDataFiles++` and `acc.addResultDeleteMetrics(task)` are unsynchronized for the same reason. So reapplying this needs decisions that are yours to make: whether residuals are computed inside each worker (needing a concurrency-safe per-spec cache) or in a sequential pass after flattening; whether splitting happens inside the worker (giving up the exact-capacity buffer) or afterwards (costing a second pass, which partly offsets what the PR is optimizing); and how metrics accumulate safely. My review and approval above still stand on the design as reviewed — this is purely about how it lands on top of what has since merged. Also note `openManifest` no longer exists, so the `scan_metrics.go` comment referencing it needs updating to match `streamManifest`. Related: #1976 is now merged and reworks `collectManifestEntriesWithSchema` into per-manifest result slots, which your delete-manifest path still uses. Worth rebasing on top of that rather than around it. Happy to review again once rebased. If you'd rather I take a pass at the merge and you review my choices, say so and I will. -- 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]
