[VARIANT] Accept variantType RFC
#4096
Conversation
There was a problem hiding this comment.
Is it safe to finalize this RFC when the parquet spec is not finalized yet?
Important
This specification is still under active development, and has not been formally adopted.
I guess this RFC actually references the spark variant spec, not the parquet variant spec -- even tho it has a whole section about "Variant data in Parquet"?
Do we need commentary in the Delta spec about it relates to those two different specifications?
|
|
||
| ## Writer Requirements for Variant Data Type | ||
|
|
||
| When Variant type is supported (`writerFeatures` field of a table's `protocol` action contains `variantType`), writers: | ||
| - must write a column of type `variant` to parquet as a struct containing the fields `value` and `metadata` and storing values that conform to the [Variant binary encoding specification](https://github.com/apache/spark/blob/master/common/variant/README.md) | ||
| - must not write additional, non-ignorable parquet struct fields. Writing additional struct fields with names starting with `_` (underscore) is allowed. | ||
| - must not write additional parquet struct fields. |
There was a problem hiding this comment.
Is there a particular reason we need to forbid extra fields in this specific case?
Normally the Delta spec just says readers should ignore any unknown fields they might encounter, because the lack of a table feature to protect such fields means they do not affect correctness.
There was a problem hiding this comment.
I don't think theres any particular reason, I think we can follow other features, i.e. modify this to say that readers can ignore fields that aren't metadata or value.
@gene-db or @cashmand do you see any issues with this? I figure this should be ok - the shredding table feature later will specify the "important" columns for shredding
There was a problem hiding this comment.
One reason to fail is that shredding adds a typed_value column. If a shredding-unaware reader doesn't fail when it encounters a column other than value and metadata, it could incorrectly read value and metadata, which might look valid, but would not contain the full value.
There was a problem hiding this comment.
@cashmad, in this case, the VariantShredding table feature would be enabled on the table though, right? So a shredding-unaware reader won't be able to read the table in the first place.
maybe i'm missing something...
There was a problem hiding this comment.
That's true. Is it possible that a customer could create a delta table using external parquet files, and we wouldn't know that we nee the shredding feature? It seems safer to me for a reader to fail.
| # Variant Data Type | ||
|
|
||
| This feature enables support for the Variant data type, for storing semi-structured data. | ||
| The schema serialization method is described in [Schema Serialization Format](#schema-serialization-format). |
There was a problem hiding this comment.
It seems a bit odd to specifically mention the schema serialization format here? AFAIK it didn't change with this table feature?
Meanwhile, it seems like we should specifically state that the new data type is called variant, rather than relying on the example below to show it?
There was a problem hiding this comment.
Ah -- the schema serialization section adds a new entry for variant, that explains the type name is variant.
| "elementType" : { | ||
| "type" : "variant" | ||
| }, | ||
| "containsNull" : false |
There was a problem hiding this comment.
According to the variant spec, a variant column could store a JSON null value even if the variant field is non-nullable as in this example.
How is that handled? Does the Delta spec need to say anything about it or is that the query engine's problem, independent of Delta?
There was a problem hiding this comment.
hmm, yeah I think this is less a storage layer issue and more of an engine issue. That should be addressed in the actual binary layout of variant (which is considered in the parquet spec)
| The parquet struct must include the two struct fields `value` and `metadata`. | ||
| Supported writers must write the two binary fields, and supported readers must read the two binary fields. | ||
|
|
||
| Variant shredding will be introduced in a separate `variantShredding` table feature. |
There was a problem hiding this comment.
Should this reference to variant shredding link to the parquet variant shredding spec? Or is that overkill?
There was a problem hiding this comment.
I think it makes sense to keep it separate for now because i imagine the parquet variant shredding spec may contain more information than is necessary for delta (i.e. the parquet binary format currently contains information for the binary encoding, which we don't include here).
I'd let @gene-db make the call here, though. Gene, do you have an opinion?
Which Delta project/connector is this regarding?
Description
moves the variant type RFC into the accepted folder and inlines it into
PROTOCOL.md.Variant has been used in production systems for many months now and has also been added as a logical type in parquet here. Additionally, Spark has a robust implementation of the variant type
Additionally specifies that variant shredding will be a different table feature and removed the portion about struct fields with
_are ignored.How was this patch tested?
manually tested that links work
Does this PR introduce any user-facing changes?