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: [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]
