Copilot commented on code in PR #13129:
URL: https://github.com/apache/gluten/pull/13129#discussion_r4317811260
##########
.github/workflows/util/delta-spark-ut/apply-delta-test-patches.sh:
##########
@@ -82,43 +83,31 @@ cherry_pick_delta_fix
46bd45d57eadd7e528002a0ae7bd36ce5a456eca "#7104 (ScanRepor
cherry_pick_delta_fix 959e00e15f41f56afc1c9bb95d160c55c6dc7068 "#7105 (9 more
test suites)"
echo "::endgroup::"
-echo "::group::Capping DeltaParquetFileFormat fixture row groups by row count"
-# DeltaParquetFileFormatSuite generates one 20,000-row Parquet file and sets a
-# 50 KiB block size to ensure that it contains multiple row groups. Velox sizes
-# row groups by buffered bytes after writing each input batch. Because this
-# fixture arrives in one batch, lowering the byte threshold cannot split it.
-# Scope Gluten's native row-count limit around the fixture write so Arrow
splits
-# the 20,000 rows deterministically while keeping the native write path
enabled.
+echo "::group::Applying temporary Delta test patches"
+# Keep local patches after the cherry-picks, which require an unmodified
worktree
+# for the files they touch. Patch paths are relative to this script, not Delta.
+for patch in \
+ cdf-pushed-filter-assertions.patch \
+ parquet-fixture-row-groups.patch \
+ dv-predicate-pushdown-row-groups.patch \
+ dv-2b-row-fail-fast.patch; do
+ echo "Applying $patch"
+ if ! git -C "$DELTA_DIR" apply "$PATCH_DIR/$patch"; then
+ echo "ERROR: Delta test patch '$patch' did not apply." >&2
+ echo "The patch expects Delta v4.4.1 test sources;" \
+ "ref '${DELTA_REF}' must remain source-compatible." >&2
+ exit 1
+ fi
+done
+echo "::endgroup::"
+
+echo "::group::Checking DeltaParquetFileFormat fixture row groups"
DPFFS="$DELTA_DIR/spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala"
if [ ! -f "$DPFFS" ]; then
echo "Expected file not found in Delta clone: $DPFFS" >&2
echo "The Delta directory layout for ref '${DELTA_REF}' may have changed."
>&2
exit 1
fi
-if ! sed 's/^__BLANK_CONTEXT__$/ /' <<'PATCH' | git -C "$DELTA_DIR" apply -
-diff --git
a/spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala
b/spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala
----
a/spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala
-+++
b/spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala
-@@ -68,9 +68,11 @@ trait DeltaParquetFileFormatSuiteBase
- protected def generateData(tablePath: String): Unit = {
- // This is to generate a Parquet file with two row groups
- hadoopConf().set("parquet.block.size", (1024 * 50).toString)
-__BLANK_CONTEXT__
- // Keep the number of partitions to 1 to generate a single Parquet data
file
- val df = Seq.range(0, 20000).toDF().repartition(1)
-- df.write.format("delta").mode("append").save(tablePath)
-+ withSQLConf("spark.gluten.sql.native.parquet.write.blockRows" -> "10000")
{
-+ df.write.format("delta").mode("append").save(tablePath)
-+ }
-__BLANK_CONTEXT__
- // Set DFS block size to be less than Parquet rowgroup size, to allow
-PATCH
-then
- echo "ERROR: DeltaParquetFileFormat fixture patch did not apply." >&2
- echo "The patch expects the Delta v4.4.1 generateData fixture shape;" \
- "ref '${DELTA_REF}' must remain source-compatible." >&2
- exit 1
-fi
ROW_CAP_SCOPES=$(
grep -Fxc \
' withSQLConf("spark.gluten.sql.native.parquet.write.blockRows" ->
"10000") {' \
Review Comment:
This verification is brittle because it requires an exact indentation match
(leading spaces) and exact formatting of the line. If upstream Delta reformats
indentation but the patch still applies correctly, setup will fail spuriously.
Consider matching the key substring without leading whitespace (or normalizing
whitespace before counting) so the check validates behavior rather than
formatting.
##########
.github/workflows/util/delta-spark-ut/apply-delta-test-patches.sh:
##########
@@ -82,43 +83,31 @@ cherry_pick_delta_fix
46bd45d57eadd7e528002a0ae7bd36ce5a456eca "#7104 (ScanRepor
cherry_pick_delta_fix 959e00e15f41f56afc1c9bb95d160c55c6dc7068 "#7105 (9 more
test suites)"
echo "::endgroup::"
-echo "::group::Capping DeltaParquetFileFormat fixture row groups by row count"
-# DeltaParquetFileFormatSuite generates one 20,000-row Parquet file and sets a
-# 50 KiB block size to ensure that it contains multiple row groups. Velox sizes
-# row groups by buffered bytes after writing each input batch. Because this
-# fixture arrives in one batch, lowering the byte threshold cannot split it.
-# Scope Gluten's native row-count limit around the fixture write so Arrow
splits
-# the 20,000 rows deterministically while keeping the native write path
enabled.
+echo "::group::Applying temporary Delta test patches"
+# Keep local patches after the cherry-picks, which require an unmodified
worktree
+# for the files they touch. Patch paths are relative to this script, not Delta.
+for patch in \
+ cdf-pushed-filter-assertions.patch \
+ parquet-fixture-row-groups.patch \
+ dv-predicate-pushdown-row-groups.patch \
+ dv-2b-row-fail-fast.patch; do
+ echo "Applying $patch"
+ if ! git -C "$DELTA_DIR" apply "$PATCH_DIR/$patch"; then
+ echo "ERROR: Delta test patch '$patch' did not apply." >&2
+ echo "The patch expects Delta v4.4.1 test sources;" \
+ "ref '${DELTA_REF}' must remain source-compatible." >&2
Review Comment:
The script hard-codes “Delta v4.4.1 test sources” in the failure message,
but the PR description mentions local validation against Delta `v4.2.0`. Please
align these (either update the message to the actually supported/ref-tested
Delta version(s), or update the PR description/docs) so users debugging patch
failures aren’t sent in the wrong direction.
##########
.github/workflows/util/delta-spark-ut/apply-delta-test-patches.sh:
##########
@@ -135,51 +124,13 @@ git -C "$DELTA_DIR" --no-pager diff -- \
"spark/src/test/scala/org/apache/spark/sql/delta/DeltaParquetFileFormatSuite.scala"
|| true
echo "::endgroup::"
-echo "::group::Capping predicate-pushdown DV fixture row groups by row count"
-# DeletionVectorsWithPredicatePushdownSuite writes one 1,000,000-row Parquet
-# file and expects its 2 MiB block size to produce two row groups. The rows can
-# arrive at Velox in one Arrow batch, so the native writer cannot evaluate its
-# buffered-byte flush threshold until the whole batch is already in one group.
-# Cap this fixture at 500,000 rows per group so Arrow splits the batch while
-# retaining the native write path and the existing Hadoop block-size setting.
+echo "::group::Checking predicate-pushdown DV fixture row groups"
DV_SUITE="$DELTA_DIR/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala"
if [ ! -f "$DV_SUITE" ]; then
echo "Expected file not found in Delta clone: $DV_SUITE" >&2
echo "The Delta directory layout for ref '${DELTA_REF}' may have changed."
>&2
exit 1
fi
-if ! sed 's/^__BLANK_CONTEXT__$/ /' <<'PATCH' | git -C "$DELTA_DIR" apply -
-diff --git
a/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala
b/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala
----
a/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala
-+++
b/spark/src/test/scala/org/apache/spark/sql/delta/deletionvectors/DeletionVectorsSuite.scala
-@@ -913,12 +913,14 @@ class DeletionVectorsWithPredicatePushdownSuite extends
DeletionVectorsSuite {
- super.beforeAll()
-__BLANK_CONTEXT__
- // 2MB rowgroups.
- hadoopConf().set("parquet.block.size", (2 * 1024 * 1024).toString)
-__BLANK_CONTEXT__
-- spark.range(0, multiRowgroupTableRowsNum, 1, 1).toDF("id")
-- .write
-- .option(DeltaConfigs.ENABLE_DELETION_VECTORS_CREATION.key,
true.toString)
-- .format("delta")
-- .saveAsTable(multiRowgroupTable)
-+ withSQLConf("spark.gluten.sql.native.parquet.write.blockRows" ->
"500000") {
-+ spark.range(0, multiRowgroupTableRowsNum, 1, 1).toDF("id")
-+ .write
-+ .option(DeltaConfigs.ENABLE_DELETION_VECTORS_CREATION.key,
true.toString)
-+ .format("delta")
-+ .saveAsTable(multiRowgroupTable)
-+ }
-__BLANK_CONTEXT__
- val deltaLog = DeltaLog.forTable(spark,
TableIdentifier(multiRowgroupTable))
- val files = deltaLog.update().allFiles.collect()
-PATCH
-then
- echo "ERROR: predicate-pushdown DV fixture patch did not apply." >&2
- echo "The patch expects the Delta v4.4.1 beforeAll fixture shape;" \
- "ref '${DELTA_REF}' must remain source-compatible." >&2
- exit 1
-fi
DV_ROW_CAP_SCOPES=$(
grep -Fxc \
' withSQLConf("spark.gluten.sql.native.parquet.write.blockRows" ->
"500000") {' \
Review Comment:
Same brittleness as the parquet fixture check: requiring exact
indentation/formatting can cause unnecessary CI breakage on harmless upstream
formatting changes. Recommend counting occurrences in a whitespace-insensitive
way (e.g., match without leading spaces and/or only the
`withSQLConf("spark.gluten.sql.native.parquet.write.blockRows"` portion).
--
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]
