[GLUTEN][VL] Re-enable the ignored "Velox Parquet Write" test in ORC suite - #12977
[GLUTEN][VL] Re-enable the ignored "Velox Parquet Write" test in ORC suite#12977yikf wants to merge 1 commit into
Conversation
baa89d2 to
f42609a
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The change re-enables the test by applying the same passing logic used in the Parquet suite, with only a minor comment wording nit noted.
Pull request overview
Re-enables the previously ignored “Velox Parquet Write” test in VeloxOrcDataTypeValidationSuite by aligning it with the already-enabled Parquet-suite version, ensuring the test no longer includes complex-type columns that Velox native write doesn’t support.
Changes:
- Switches the test from
ignoretotestso it runs in the ORC suite again. - Drops complex-type columns (
array,struct,map) before writing Parquet with native writer enabled. - Adds a read-back +
checkAnswerverification to validate write correctness.
File summaries
| File | Description |
|---|---|
| backends-velox/src/test/scala/org/apache/gluten/execution/VeloxOrcDataTypeValidationSuite.scala | Re-enables the Parquet write test and adjusts the input DataFrame to avoid unsupported complex types; verifies write/read correctness. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to re-enabling a test with a straightforward fix and added validation, with only minor naming cleanup suggested.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
backends-velox/src/test/scala/org/apache/gluten/execution/VeloxOrcDataTypeValidationSuite.scala:474
- New local variables use snake_case (
write_path,data_path), which is inconsistent with the rest of this suite’s Scala naming (e.g.,executedPlan) and typical camelCase conventions. Renaming improves readability and avoids introducing a one-off style exception in this file.
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@zhztheplayer could you please take a look, thanks! |
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| val write_path = dir.toURI.getPath | ||
| val data_path = getClass.getResource("/").getPath + "/data-type-validation-data/type1" |
| val parquetDf = spark.read | ||
| .format("parquet") | ||
| .load(write_path) | ||
| checkAnswer(parquetDf, df) |
| val write_path = dir.toURI.getPath | ||
| val data_path = getClass.getResource("/").getPath + "/data-type-validation-data/type1" |
| withSQLConf((GlutenConfig.NATIVE_WRITER_ENABLED.key, "true")) { | ||
| withTempDir { | ||
| dir => | ||
| val write_path = dir.toURI.getPath |
There was a problem hiding this comment.
Thanks, please rename write_path and data_path to writePath and dataPath to follow Scala’s lowerCamelCase naming convention.
7a84b07 to
8ef69aa
Compare
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
| val writePath = dir.toURI.getPath | ||
| val dataPath = getClass.getResource("/").getPath + "/data-type-validation-data/type1" |
| df.write.mode("append").format("parquet").save(write_path) | ||
| val writePath = dir.toURI.getPath | ||
| val dataPath = getClass.getResource("/").getPath + "/data-type-validation-data/type1" | ||
| // Velox native write doesn't support Complex type. |
| .drop("array") | ||
| .drop("struct") | ||
| .drop("map") |
What changes are proposed in this pull request?
The "Velox Parquet Write" test in VeloxOrcDataTypeValidationSuite has been ignored since the Spark 3.4 test framework was introduced.
The identical test in VeloxParquetDataTypeValidationSuite is enabled and passing, the only reason the ORC-suite copy failed was that it did not drop the complex-type columns that Velox native write does not support.
How was this patch tested?
re-enable test.
Was this patch authored or co-authored using generative AI tooling?
Yes, Generated-by: Claude claude-opus-4-8