You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "harshmotw-db (via GitHub)" <gi...@apache.org> on 2025/11/11 21:43:03 UTC
[PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
harshmotw-db opened a new pull request, #53005:
URL: https://github.com/apache/spark/pull/53005
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
This PR makes changes to the parquet writer to make it annotate variant columns with the parquet variant logical type annotation.
### Why are the changes needed?
The Parquet spec has formally adopted the Variant logical type, and therefore, Variant columns must be properly annotated in Spark 4.1.0 which depends on Parquet-java 1.16.0 which contains the variant logical type annotation.
This change is hidden behind a flag that is disabled by default until read support can be properly implemented.
### Does this PR introduce _any_ user-facing change?
Yes, Parquet files written by Spark 4.1.0 with the flag enabled (which it eventually will be by default) could contain the variant logical type annotation which readers without support for the type will not be able to read
### How was this patch tested?
Unit test to check if nested as well as top-level variants are properly annotated, and the data is being written correctly.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #53005:
URL: https://github.com/apache/spark/pull/53005#discussion_r2516214340
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1585,6 +1585,21 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val PARQUET_ANNOTATE_VARIANT_LOGICAL_TYPE =
+ buildConf("spark.sql.parquet.variant.annotateLogicalType.enabled")
+ .doc("When enabled, Spark annotates the variant groups written to Parquet as the parquet " +
+ "variant logical type.")
+ .version("4.1.0")
+ .booleanConf
+ .createWithDefault(false)
+
+ val PARQUET_WRITE_VARIANT_SPEC_VERSION =
Review Comment:
Actually, yeah I agree. It shouldn't be based on a config. I'll fix it
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "cashmand (via GitHub)" <gi...@apache.org>.
cashmand commented on code in PR #53005:
URL: https://github.com/apache/spark/pull/53005#discussion_r2516030733
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaConverter.scala:
##########
@@ -373,6 +373,10 @@ class ParquetToSparkSchemaConverter(
Option(field.getLogicalTypeAnnotation).fold(
convertInternal(groupColumn, sparkReadType.map(_.asInstanceOf[StructType]))) {
+ // Temporary workaround to read Shredded variant data
+ case v: VariantLogicalTypeAnnotation if v.getSpecVersion == 1 && sparkReadType.isEmpty =>
+ convertInternal(groupColumn, None)
Review Comment:
Is the entire read-side PR just this and one or two other similar lines, or is there something else that I'm missing?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "cashmand (via GitHub)" <gi...@apache.org>.
cashmand commented on code in PR #53005:
URL: https://github.com/apache/spark/pull/53005#discussion_r2516030733
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaConverter.scala:
##########
@@ -373,6 +373,10 @@ class ParquetToSparkSchemaConverter(
Option(field.getLogicalTypeAnnotation).fold(
convertInternal(groupColumn, sparkReadType.map(_.asInstanceOf[StructType]))) {
+ // Temporary workaround to read Shredded variant data
+ case v: VariantLogicalTypeAnnotation if v.getSpecVersion == 1 && sparkReadType.isEmpty =>
+ convertInternal(groupColumn, None)
Review Comment:
Is the entire read-side PR just this and one or two other similar lines, or is there something else that I'm missing?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -1585,6 +1585,21 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val PARQUET_ANNOTATE_VARIANT_LOGICAL_TYPE =
+ buildConf("spark.sql.parquet.variant.annotateLogicalType.enabled")
+ .doc("When enabled, Spark annotates the variant groups written to Parquet as the parquet " +
+ "variant logical type.")
+ .version("4.1.0")
+ .booleanConf
+ .createWithDefault(false)
+
+ val PARQUET_WRITE_VARIANT_SPEC_VERSION =
Review Comment:
It seems a bit strange for this to be a conf for now. I don't think we should allow writing a version that Spark doesn't know how to write, and currently the only valid spec version is 1, so if we have a conf, it should only accept "1" as a valid setting. Is there some use for this conf that I'm missing?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #53005: [SPARK-54306] Annotate Variant columns with Variant logical type annotation
URL: https://github.com/apache/spark/pull/53005
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on code in PR #53005:
URL: https://github.com/apache/spark/pull/53005#discussion_r2515926348
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/parquet/ParquetSchemaConverter.scala:
##########
@@ -373,6 +373,10 @@ class ParquetToSparkSchemaConverter(
Option(field.getLogicalTypeAnnotation).fold(
convertInternal(groupColumn, sparkReadType.map(_.asInstanceOf[StructType]))) {
+ // Temporary workaround to read Shredded variant data
+ case v: VariantLogicalTypeAnnotation if v.getSpecVersion == 1 && sparkReadType.isEmpty =>
+ convertInternal(groupColumn, None)
Review Comment:
@chenhao-db I have added this temporary workaround on the reader side to allow tests to be able to scan a table containing Variant data if the `VARIANT_ALLOW_READING_SHREDDED` config is set to true
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "harshmotw-db (via GitHub)" <gi...@apache.org>.
harshmotw-db commented on PR #53005:
URL: https://github.com/apache/spark/pull/53005#issuecomment-3518856693
@cashmand @chenhao-db @cloud-fan Can you please look at this PR? Thanks!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-54306] Annotate Variant columns with Variant logical type annotation [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #53005:
URL: https://github.com/apache/spark/pull/53005#issuecomment-3522604271
thanks, merging to master/4.1!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org