-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-53659][SQL] Infer Variant shredding schema when writing to Parquet #52406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gene-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cashmand Thanks for this awesome feature! I left a few comments.
| /** | ||
| * | ||
| * Infer a schema when there are Variant values in the shredding schema. | ||
| * Only VariantType values at the top level or nested in struct fields are replaced. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are they replaced with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant replacing VariantType in the schema with the shredding schema, but I'll change the comment to be are shredded.
| * Infer a schema when there are Variant values in the shredding schema. | ||
| * Only VariantType values at the top level or nested in struct fields are replaced. | ||
| * VariantType nested in arrays or maps are not modified. | ||
| * @param schema The original schema containing VariantType. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require that the schema must contain a VariantType, and does the VariantType have to be a top-level field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it operates on nested struct fields as well, and it's fine to call it if there's no VariantType at all in the schema, it will just be a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll change the description to be The original schema, with no shredding.
| .flatten | ||
| } | ||
|
|
||
| private def getValueAtPath(s: StructType, row: InternalRow, p: Seq[Int]): Option[VariantVal] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT:
| private def getValueAtPath(s: StructType, row: InternalRow, p: Seq[Int]): Option[VariantVal] = { | |
| private def getValueAtPath(schema: StructType, row: InternalRow, path: Seq[Int]): Option[VariantVal] = { |
Also, can we comment on what this will return? It looks it will return None of the field will be null?
| } | ||
|
|
||
| /** | ||
| * Update each VariantType with its inferred schema. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it return a copy of the schema (with the updates), or does it update in place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The former. I'll change the comment to Return a new schema, with each VariantType replaced its inferred shredding schema.
gene-db
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this feature!
LGTM
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
Outdated
Show resolved
Hide resolved
|
The last commit just update the config version doc, no need to wait for CI, thanks, merging to master! |
|
@cloud-fan @gene-db @cashmand, currently, variant is guarded by several internal configs, do you plan to enable variant support in Spark 4.1? |
Hi @pan3793, I think the only remaining item is to update the parquet reader and writer to recognize and write the Variant logical type annotation that was released in the latest version of parquet-java, and then we should be able to enable it by default. |
|
@cashmand, thanks for the information, sounds promising, I hope it can happen in 4.1 |
…quet ### What changes were proposed in this pull request? When writing Variant to Parquet, we want the shredding schema to adapt to the data being written on a per-file basis. This PR adds a new output writer that buffers the first few rows before starting the write, then uses the content of those rows to determine a shredding schema, and only then creates the Parquet writer with that schema. The heuristics for determining the shredding schema are currently fairly simple: if a field appears consistently with a consistent type, we create `value` and `typed_value`, and if it appears with an inconsistent type, we only create `value`. We drop fields that occur in less than 10% of sampled rows, and have an upper bound of 300 total fields (counting `value` and `typed_value` separately) to avoid creating excessively wide Parquet schemas, which can cause performance issues. ### Why are the changes needed? Allows Spark to make use of the [Variant shredding spec](https://github.com/apache/parquet-format/blob/master/VariantShredding.md) without requiring the user to manually set a shredding schema. ### Does this PR introduce _any_ user-facing change? Only if `spark.sql.variant.inferShreddingSchema` and `spark.sql.variant.writeShredding.enabled` are both set to true. They currently false by default. ### How was this patch tested? Unit tests in PR. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#52406 from cashmand/variant_shredding_inference. Lead-authored-by: cashmand <david.cashman@databricks.com> Co-authored-by: Wenchen Fan <cloud0fan@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
When writing Variant to Parquet, we want the shredding schema to adapt to the data being written on a per-file basis. This PR adds a new output writer that buffers the first few rows before starting the write, then uses the content of those rows to determine a shredding schema, and only then creates the Parquet writer with that schema.
The heuristics for determining the shredding schema are currently fairly simple: if a field appears consistently with a consistent type, we create
valueandtyped_value, and if it appears with an inconsistent type, we only createvalue. We drop fields that occur in less than 10% of sampled rows, and have an upper bound of 300 total fields (countingvalueandtyped_valueseparately) to avoid creating excessively wide Parquet schemas, which can cause performance issues.Why are the changes needed?
Allows Spark to make use of the Variant shredding spec without requiring the user to manually set a shredding schema.
Does this PR introduce any user-facing change?
Only if
spark.sql.variant.inferShreddingSchemaandspark.sql.variant.writeShredding.enabledare both set to true. They currently false by default.How was this patch tested?
Unit tests in PR.
Was this patch authored or co-authored using generative AI tooling?
No.