You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "sezruby (via GitHub)" <gi...@apache.org> on 2026/08/12 18:18:22 UTC

[PR] [VL] Support parquet.enable.page.index write option [gluten]

sezruby opened a new pull request, #12760:
URL: https://github.com/apache/gluten/pull/12760

   ## What changes are proposed in this pull request?
   
   Add a `parquet.enable.page.index` write option that maps to Velox's
   `ParquetWriterOptions::enableWritePageIndex` (facebookincubator/velox#18325),
   mirroring the existing `parquet.enable.dictionary` passthrough. When enabled,
   the native Parquet writer emits the column index and offset index (page index);
   when disabled, they are omitted.
   
   - `GlutenConfig` (Scala and C++): define the `parquet.enable.page.index` key.
   - `VeloxParquetWriterInjects`: forward the option into the native write config.
   - `VeloxWriterUtils::makeParquetWriteOption`: set `enableWritePageIndex`.
   - Document the option in the Parquet write configuration table.
   
   This also bumps the pinned Velox from `dft-2026_08_05` to `dft-2026_08_12`, the
   first snapshot that includes facebookincubator/velox#18325 (the
   `enableWritePageIndex` field).
   
   ## How was this patch tested?
   
   Added a `VeloxParquetWriteSuite` test that writes with `parquet.enable.page.index`
   set to both `true` and `false`, asserts (via parquet-mr footer inspection) that
   the Parquet `ColumnIndex`/`OffsetIndex` references are present exactly when the
   option is enabled, and verifies the written data round-trips.
   
   Ran the test locally against a Velox build that includes the
   `enableWritePageIndex` field; it passes — the page index is written when the
   option is enabled and omitted when disabled.
   
   ## Was this patch authored or co-authored using generative AI tooling?
   
   Generated-by: Claude Code (Claude Opus 4.8)
   


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5271028575

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "zhouyuan (via GitHub)" <gi...@apache.org>.
zhouyuan commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5294901578

   @sezruby would you please do a rebase? The parquet index patch is already landed


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "sezruby (via GitHub)" <gi...@apache.org>.
sezruby commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5319818117

   Rebased onto the latest main as requested, @zhouyuan — CI is now fully green.
   
   @infvg I've addressed your review comments on the test: removed the extra comments and split the column-index / offset-index checks into two separate asserts, so a regression that breaks only one of them is still caught. PTAL when you get a chance.
   
   For context on the earlier red CI here: those failures were transient GitHub rate-limiting (HTTP 429) while fetching build dependencies during the CMake configure step, not a code issue. I opened #12804 to add a short retry/backoff around that step so this class of transient download failure no longer fails the build.
   


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5318464481

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5295552674

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5270934135

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "infvg (via GitHub)" <gi...@apache.org>.
infvg commented on code in PR #12760:
URL: https://github.com/apache/gluten/pull/12760#discussion_r3796533243


##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/VeloxParquetWriteSuite.scala:
##########
@@ -306,6 +306,57 @@ class VeloxParquetWriteSuite extends VeloxWholeStageTransformerSuite with WriteU
         checkAnswer(parquetDf, spark.range(100).toDF("id"))
     }
   }
+
+  test("test write parquet with page index enabled/disabled/default") {
+    // Page index (column index + offset index) write path. The native writer maps
+    // parquet.enable.page.index -> Velox ParquetWriterOptions.enableWritePageIndex
+    // (facebookincubator/velox#18325) and defaults it on to match Spark/parquet-mr
+    // (SPARK-26345). Small page size forces many pages so the page index is
+    // materially written when enabled. None => option unset, exercising the default.
+    Seq(Some(true), Some(false), None).foreach {
+      enablePageIndex =>
+        withTempPath {
+          f =>
+            val writer = spark
+              .range(0, 100000, 1, 1)
+              .selectExpr("id", "cast(id % 100 as int) as v")
+              .write
+              .format("parquet")
+              .option(GlutenConfig.PARQUET_DATAPAGE_SIZE, (4 * 1024).toString)
+            enablePageIndex.foreach(
+              v => writer.option(GlutenConfig.PARQUET_ENABLE_PAGE_INDEX, v.toString))
+            writer.save(f.getCanonicalPath)
+
+            // Unset defaults to on.

Review Comment:
   Please remove this comment



##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/VeloxParquetWriteSuite.scala:
##########
@@ -306,6 +306,57 @@ class VeloxParquetWriteSuite extends VeloxWholeStageTransformerSuite with WriteU
         checkAnswer(parquetDf, spark.range(100).toDF("id"))
     }
   }
+
+  test("test write parquet with page index enabled/disabled/default") {
+    // Page index (column index + offset index) write path. The native writer maps
+    // parquet.enable.page.index -> Velox ParquetWriterOptions.enableWritePageIndex
+    // (facebookincubator/velox#18325) and defaults it on to match Spark/parquet-mr
+    // (SPARK-26345). Small page size forces many pages so the page index is
+    // materially written when enabled. None => option unset, exercising the default.

Review Comment:
   This comment feels needlessly wordy, it can be removed



##########
backends-velox/src/test/scala/org/apache/spark/sql/execution/VeloxParquetWriteSuite.scala:
##########
@@ -306,6 +306,57 @@ class VeloxParquetWriteSuite extends VeloxWholeStageTransformerSuite with WriteU
         checkAnswer(parquetDf, spark.range(100).toDF("id"))
     }
   }
+
+  test("test write parquet with page index enabled/disabled/default") {
+    // Page index (column index + offset index) write path. The native writer maps
+    // parquet.enable.page.index -> Velox ParquetWriterOptions.enableWritePageIndex
+    // (facebookincubator/velox#18325) and defaults it on to match Spark/parquet-mr
+    // (SPARK-26345). Small page size forces many pages so the page index is
+    // materially written when enabled. None => option unset, exercising the default.
+    Seq(Some(true), Some(false), None).foreach {
+      enablePageIndex =>
+        withTempPath {
+          f =>
+            val writer = spark
+              .range(0, 100000, 1, 1)
+              .selectExpr("id", "cast(id % 100 as int) as v")
+              .write
+              .format("parquet")
+              .option(GlutenConfig.PARQUET_DATAPAGE_SIZE, (4 * 1024).toString)
+            enablePageIndex.foreach(
+              v => writer.option(GlutenConfig.PARQUET_ENABLE_PAGE_INDEX, v.toString))
+            writer.save(f.getCanonicalPath)
+
+            // Unset defaults to on.
+            val expectPageIndex = enablePageIndex.getOrElse(true)
+            val parquetFiles = f.list((_, name) => name.contains("parquet"))
+            assert(parquetFiles.nonEmpty)
+            val hasPageIndex = parquetFiles.exists {
+              file =>
+                val path = new Path(f.getCanonicalPath, file)
+                val in = HadoopInputFile.fromPath(path, spark.sessionState.newHadoopConf())
+                Utils.tryWithResource(ParquetFileReader.open(in)) {
+                  reader =>
+                    reader.getFooter.getBlocks.asScala.exists {
+                      block =>
+                        block.getColumns.asScala.exists {
+                          col =>
+                            col.getColumnIndexReference != null ||
+                            col.getOffsetIndexReference != null

Review Comment:
   Can we assert these separately? Both of these should be produced so if a regression occurs that breaks one we won't notice
   



-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "zhouyuan (via GitHub)" <gi...@apache.org>.
zhouyuan merged PR #12760:
URL: https://github.com/apache/gluten/pull/12760


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5317279817

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org


Re: [PR] [VL] Support parquet.enable.page.index write option [gluten]

Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12760:
URL: https://github.com/apache/gluten/pull/12760#issuecomment-5295414862

   Run Gluten Clickhouse CI on x86


-- 
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: commits-unsubscribe@gluten.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org