[Spark] DV Reads Stability Improvement in Delta by removing Broadcasting DV Information

[longvu-db]

Which Delta project/connector is this regarding?

• Spark
• Standalone
• Flink
• Kernel
• Other (fill in here)

Description

Back then, we relied on an expensive Broadcast of DV files to pass the DV files to the associated Parquet Files. With the introduction of adding custom metadata to files introduced in Spark 3.5, we can now pass the DV through the custom metadata field, this is expected to improve the stability of DV reads in Delta.

How was this patch tested?

Adjusted the existing UTs that cover our changes.

Does this PR introduce any user-facing changes?

No.

[xupefei]

Very nice, thank you! Looking forward to seeing how much speed-up we can get with this change :)

[xupefei]

metadata shows the current status of the table, i.e., if DV is enabled in the current version, isn't it? How about some old versions that have DV, but now the user has decided to disable it? I think we should look at the table protocol: if DV table feature is turned on, then we assume the table contains DV.

[longvu-db]

@xupefei Quick question, can an existing table add new table feature? Like if a table does not have DV from the beginning, in the property, and then at some point later in time get added DVs.

[longvu-db]

@xupefei I think you can do this using ALTER TABLE right? But once it is in, you cannot drop it at the moment.

[xupefei]

After looking at the code, I feel that we need a test case with the following logic:

1. Create a table without DV. Insert some values.
2. Turn on DV (delta.enableDeletionVectors = true).
3. Delete some values.
4. Turn off DV (delta.enableDeletionVectors = false).
5. Delete some values.

Check:

1. Version 1 does not contain DV metadata column.
2. Version 2 does not contain DV metadata column (because there's no DV in this table - correct me if I'm wrong).
3. Version 3 does contain DV metadata columns.
4. Version 4 does contain DV metadata columns.
5. Version 5 does contain DV metadata columns.

[longvu-db]

@xupefei I addressed your comments =)).

Regarding the new test case:

1. Version 2 should contain DV metadata column (because having the metadata columns depends on whether or not the table's protocol supports DV, not on whether any DVs is written or not)
2. What is the value of doing action 5? If we disable the DV for the table metadata, and if the DV metadata indeed stay, then whatever actions we do, it would always stay right?

[xupefei]

A few improvements to the tests then we're good to go

[xupefei]

Cc @larsk-db

[johanl-db]

I think you can get rid of broadcastHadoopConf and hasBroadcastHadoopConf altogether here.

The only use we have for it now is to pass it to rowIndexFilter.createInstance when creating the DV filter but we also have hadoopConf: Configuration around (passed as an argument to buildReaderWithPartitionValues) which I believe should be sufficient here: we don't need this configuration to be broadcast first for the purpose of loading DVs from storage

[longvu-db]

@johanl-db No clue why we had it back in the days hmm

[longvu-db]

@johanl-db Removing broadcastHadoopConf doesn't seem to work, I haven't investigated yet, changing back to using the broadcastHadoopConf makes the tests passed

[larsk-db]

Cc @larsk-db

I took a quick look, but it seems you and @johanl-db are on top of this. Ping me if you have something specific that needs my input.

[tdas]

I highly approve this change :D
Thank you for doing this! This will improve the stability on large tables with lots of DVs significantly.