Copilot commented on code in PR #12866: URL: https://github.com/apache/gluten/pull/12866#discussion_r4419891459
########## .github/workflows/delta_spark_ut.yml: ########## @@ -139,10 +139,8 @@ env: # fixed setup is paid 8 times instead of 4). # # Memory is per runner and therefore unaffected: each shard is its own job, so - # it is still 4 forks plus the sbt launcher against the ~16G limit, at ~4G per - # fork (2G heap + 2G off-heap). That fits because the worst memory hog - # (DeletionVectorsSuite 2B-row) is force-failed in - # apply-delta-test-patches.sh. + # it is still 4 forks plus the sbt launcher against the ~16G limit, with each + # fork configured for 2G heap + 2G off-heap. Review Comment: With the 2B-row cases no longer force-failed, they can execute in the existing four-way fork pool, where each fork is configured for 2 GiB heap plus 2 GiB off-heap and the sbt launcher also consumes memory. That combination can exceed the runner's ~16 GiB limit before native overhead; the historical failure reached roughly 13 GiB RSS in one fork and was OOM-killed. Please isolate these cases in a lower-parallelism shard or retain the guard until their memory use is bounded, otherwise a native OOM can kill or wedge the whole shard instead of producing a test result. ########## .github/workflows/util/delta-spark-ut/apply-delta-test-patches.sh: ########## @@ -90,8 +90,7 @@ for patch in \ cdf-pushed-filter-assertions.patch \ mock-s3-fixture-writer.patch \ parquet-fixture-row-groups.patch \ - dv-predicate-pushdown-row-groups.patch \ - dv-2b-row-fail-fast.patch; do + dv-predicate-pushdown-row-groups.patch; do Review Comment: Removing the fail-fast patch re-enables the exact unbounded path tracked by #12387, but the evidence in this PR does not establish that it is safe: the original `DeletionVectorsSuite` was not present in the executed-test inventory, while the inherited predicate-pushdown cases fell back to Spark and suppressed Spark's `More than Int.MaxValue elements` error. A real native execution can therefore still reproduce the historical ~13 GiB RSS/OOM and wedge the shard before the watchdog recovers it. Keep the guard, or remove it only after a bounded run of both original read/delete cases and their inherited cases with native plans confirmed. ########## .github/workflows/util/delta-spark-ut/README.md: ########## @@ -44,22 +44,18 @@ starts failing** (a regression). | `java-test-args.sh` | Shared Gluten Spark defaults and JVM flags (`--add-opens` + Netty property). Sourced by `run-delta-tests.sh` and by local runs. | | `setup-delta.sh` | Clones Delta, drops in the Gluten bundle, and applies temporary compatibility patches. Takes `<delta_ref> <delta_dir> <gluten_bundle_jar>`. | | `apply-delta-test-patches.sh` | Cherry-picks upstream scan fixes, applies the ordered local patch list, and verifies the resulting test sources. Called by `setup-delta.sh` with `<delta_ref> <delta_dir>`. | -| `patches/*.patch` | Separate patches for CDF plan assertions, mock-S3 fixture writes, the two Parquet row-group fixtures, and 2B-row DV fail-fast behavior. Each patch documents its rationale and removal condition. | +| `patches/*.patch` | Separate patches for CDF plan assertions, mock-S3 fixture writes, and the two Parquet row-group fixtures. Each patch documents its rationale and removal condition. | Review Comment: The documentation update is incomplete: `docs/backends/velox/get-started/VeloxDelta.md` still says that this job force-fails the two 2-billion-row tests and describes all four entries as force-failed (`VeloxDelta.md:44,46,436`). After removing the patch and baseline entries here, those statements are false and will mislead users about the current CI coverage and issue status. Update those references in the same change. ########## .github/workflows/util/delta-spark-ut/known-failures.txt: ########## @@ -340,8 +340,6 @@ org.apache.spark.sql.delta.SnapshotManagementWithCatalogManagedBatch2Suite#recov org.apache.spark.sql.delta.SnapshotManagementWithCatalogManagedBatch2Suite#should not recover when both the current and previous checkpoints are broken org.apache.spark.sql.delta.UpdateMetricsSuite#update-metrics: update one row per file - Partitioned = false, cdfEnabled = false org.apache.spark.sql.delta.coordinatedcommits.CoordinatedCommitsSuite#Incomplete backfills are handled properly by next commit after CC to FS conversion -org.apache.spark.sql.delta.deletionvectors.DeletionVectorsSuite#huge table: delete a small number of rows from tables of 2B rows with DVs -org.apache.spark.sql.delta.deletionvectors.DeletionVectorsSuite#huge table: read from tables of 2B rows with existing DV of many zeros org.apache.spark.sql.delta.deletionvectors.DeletionVectorsWithPredicatePushdownSuite#PredicatePushdown: Multiple delete statements. vectorizedReaderEnabled: false readColumnarBatchAsRows: false Review Comment: These baseline entries are being removed even though the PR reports that `DeletionVectorsSuite` did not appear in the executed-test inventories. The aggregate gate only reports tests absent from `union_ran` as stale; it does not fail or require their presence, so removing the entries can silently turn an unselected/missing suite into apparent coverage while providing no evidence that the read/delete cases ran. Retain all four entries until the suite/test inventory confirms execution, or add an explicit assertion that these four cases were run. -- 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]
