[Protocol] Add delta.writePartitionColumnsToParquet to tblproperties

[itsbilal]

Skipping RFC process as it's a backward-compatible, best-effort change (see #6324 for precedence)

Which Delta project/connector is this regarding?

• Spark
• Standalone
• Flink
• Kernel
• Other (Protocol)

Description

This change adds a definition for the
delta.writePartitionColumnsToParquet table property to the list of table properties. This property controls the materialization of partition columns in data parquet files, in the absence of any writer features that would require materialization of partition columns (and therefore take precedence over this property).

How was this patch tested?

N/a

Does this PR introduce any user-facing changes?

No

[sanujbasu]

1/ I don't see a section which explains how this relates to https://github.com/delta-io/delta/blob/master/protocol_rfcs/materialize-partition-columns.md

2/ I think this needs to be structured closer to the section which talks about serializing partition values or in a separate section with the serialization section linking to the new section.

Its hard to read this and understand what the algorithm/ behavior would be on the Kernel/ connection side? Maybe have a truth table?

[itsbilal]

It does reference writer features that govern materialization of partition values, which are multiple (IcebergCompatV1, IcebergCompatV2, IcebergCompatV3, IcebergWriterCompatV1, IcebergWriterCompatV3, materializePartitionColumns), and the writer features are treated as one set that are prioritized over this property. I don't see why we should talk about one writer feature in isolation.

2/ I think this needs to be structured closer to the section which talks about serializing partition values or in a separate section with the serialization section linking to the new section.

The protocol doesn't spell out materialization of partition values at all anywhere else, with the exception of the above-listed writer features. We could do it if we want but we would then need to decide if there's a default stance the writers should take.

Its hard to read this and understand what the algorithm/ behavior would be on the Kernel/ connection side? Maybe have a truth table?

For this property, should I just reword it to "this property only governs materialization of partition columns if no writer feature requiring materialization of partition columns is enabled on the table" - that's the same point as the existing text with fewer words, and still leaves enough ambiguity for either default true / default false.

[sanujbasu]

1/ We might not need to talk about one feature in isolation. But materialization of partition values is an entire functional area. I think we need a top level section covering all features for which we materialize partition values in addition to https://github.com/sanujbasu/delta/blob/e4c1fa465d51a02355b78366151818ffaee342a7/PROTOCOL.md#partition-value-serialization.

2/ "this property only governs materialization of partition columns if no writer feature requiring materialization of partition columns is enabled on the table" is good phrasing but it hard to understand why "materializePartitionColumns" alone doesnt address that use case since its enabled by default?
Also there is no mention of materializePartitionColumns in the protocol.md? Everyone ive discussed this with is confused about why we need the toggle if we already have a feature? If the toggle is to add the materializePartitionColumns writer feature to the list, why not just add the feature to the vector?

[itsbilal]

1/ We might not need to talk about one feature in isolation. But materialization of partition values is an entire functional area. I think we need a top level section covering all features for which we materialize partition values in addition to https://github.com/sanujbasu/delta/blob/e4c1fa465d51a02355b78366151818ffaee342a7/PROTOCOL.md#partition-value-serialization.

That one is about how they're serialized in AddFile though. In parquet we don't always use the string type, it's just the same data type that's used if it were any other column.

I've made some references to the optionality of the presence of partition values in data files in this PR now, let me know how that sounds. I'm wary of writing up the whole decision tree in protocol because not all writers will be expected to support iceberg compat, etc., and the table property spec makes it pretty clear that the toggle is best-effort. Really the only thing the protocol should enforce is "readers should understand both states, writers are sometimes required to write partition values".

2/ "this property only governs materialization of partition columns if no writer feature requiring materialization of partition columns is enabled on the table" is good phrasing but it hard to understand why "materializePartitionColumns" alone doesnt address that use case since its enabled by default?
Also there is no mention of materializePartitionColumns in the protocol.md? Everyone ive discussed this with is confused about why we need the toggle if we already have a feature? If the toggle is to add the materializePartitionColumns writer feature to the list, why not just add the feature to the vector?

materializePartitionColumns is not in the protocol because it's just an RFC that hasn't been merged yet. (funny enough, we did merge it into the protocol... and then reverted it because it hadn't baked as an RFC for long enough yet)

Everyone ive discussed this with is confused about why we need the toggle if we already have a feature?

Features block writes if they're present in a protocol and a writer doesn't understand them. We want the ability to materialize partition columns without blocking writers, especially since this is a new writer feature and we don't want to block writers all the time but rather we just want a best-effort way to require writers to materialize partition values.

If you're suggesting that we go ahead and default to writing partition values all the time, I'm supportive of a change like that. I just think that in that reality we would still need the ability to configure that behaviour, and for that we'd need a table property.

[itsbilal]

Based on an offline conversation, we decided to move this change to the materialize partition columns RFC, along with a detailed description of when and when not to materialize partition columns. Opened a separate PR for that: #6738