[VL] Support parquet.enable.page.index write option - #12760
Conversation
|
Run Gluten Clickhouse CI on x86 |
3355a00 to
a5a5fcf
Compare
|
Run Gluten Clickhouse CI on x86 |
|
@sezruby would you please do a rebase? The parquet index patch is already landed |
a5a5fcf to
95a1ca3
Compare
|
Run Gluten Clickhouse CI on x86 |
95a1ca3 to
ce5a0b5
Compare
|
Run Gluten Clickhouse CI on x86 |
infvg
left a comment
infvg
left a comment
There was a problem hiding this comment.
LGTM, just a few minor nits. Also please run ./dev/gen-all-config-docs.sh if you haven't already
Add a parquet.enable.page.index write option that maps to the Velox ParquetWriterOptions::enableWritePageIndex field (facebookincubator/velox#18325): - GlutenConfig (Scala/C++): define the parquet.enable.page.index key. - VeloxParquetWriterInjects: forward the option to native write config. - VeloxWriterUtils::makeParquetWriteOption: set enableWritePageIndex, defaulting to on when the option is unset. When enabled, the writer emits the Parquet column index and offset index (page index); when disabled, they are omitted. The option defaults to true to match Spark/parquet-mr, which write the page index by default (SPARK-26345); Velox's writer leaves it opt-in. Set parquet.enable.page.index=false to turn it off. Documented in the parquet write configuration table and covered by a VeloxParquetWriteSuite test that asserts the ColumnIndex/OffsetIndex presence tracks the option (enabled, disabled, and unset/default) and that data round-trips. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ce5a0b5 to
f998f93
Compare
|
Run Gluten Clickhouse CI on x86 |
|
Run Gluten Clickhouse CI on x86 |
|
@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. |
3 participants
What changes are proposed in this pull request?
Add a
parquet.enable.page.indexwrite option that maps to Velox'sParquetWriterOptions::enableWritePageIndex(facebookincubator/velox#18325).When enabled, the native Parquet writer emits the column index and offset index
(page index); when disabled, they are omitted.
The option defaults to
true, matching Spark/parquet-mr, which write the pageindex by default (SPARK-26345); Velox's writer leaves it opt-in. Set
parquet.enable.page.index=falseto turn it off.GlutenConfig(Scala and C++): define theparquet.enable.page.indexkey.VeloxParquetWriterInjects: forward the option into the native write config.VeloxWriterUtils::makeParquetWriteOption: setenableWritePageIndex,defaulting to on when the option is unset.
How was this patch tested?
Added a
VeloxParquetWriteSuitetest that writes withparquet.enable.page.indexset to
true, set tofalse, and left unset, and asserts (via parquet-mr footerinspection) that the Parquet
ColumnIndex/OffsetIndexreferences are presentwhen the option is on or defaulted and absent when it is off, and that the written
data round-trips.
The enabled and disabled write paths were validated end-to-end in a Docker build
against a Velox snapshot that includes the
enableWritePageIndexfield; thedefault (unset) path sets the same
enableWritePageIndex=trueas theexplicit-enable path.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)