Issue #809: Fix Creating an external table options #820

nflucian · 2021-11-01T09:17:30Z

This PR fix metadata.configuration inconsistency between write into delta and create delta table. #809

allisonport-db · 2021-11-12T21:50:33Z

Hi Lucian, thank you for contributing and submitting this PR. This is definitely a bug we want to fix and we are reviewing it now.

zsxwing · 2021-11-17T08:17:27Z

Thanks for making the PR. metadata.configuration is supposed to store the table properties rather than options. Currently we forget to remove options from table properties here.

Could you just change the above place to split table properties and options properly? Similar to

delta/core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaCatalog.scala

Lines 354 to 368 in d9e3877

    
           val props = new util.HashMap[String, String]() 
        
           // Options passed in through the SQL API will show up both with an "option." prefix and 
        
           // without in Spark 3.1, so we need to remove those from the properties 
        
           val optionsThroughProperties = properties.asScala.collect { 
        
             case (k, _) if k.startsWith("option.") => k.stripPrefix("option.") 
        
           }.toSet 
        
           val sqlWriteOptions = new util.HashMap[String, String]() 
        
           properties.asScala.foreach { case (k, v) => 
        
             if (!k.startsWith("option.") && !optionsThroughProperties.contains(k)) { 
        
               // Do not add to properties 
        
               props.put(k, v) 
        
             } else if (optionsThroughProperties.contains(k)) { 
        
               sqlWriteOptions.put(k, v) 
        
             } 
        
           }

zsxwing · 2021-12-02T19:54:31Z

core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaCatalog.scala

+      val props = if (conf.getConf(DeltaSQLConf.DELTA_LEGACY_STORE_WRITER_OPTIONS_AS_PROPS)) {
        // Legacy behavior
-        writeOptions.foreach { case (k, v) => props.put(k, v) }
+        catalogProp.properties ++ writeOptions
      } else {
-        writeOptions.foreach { case (k, v) =>
-          // Continue putting in Delta prefixed options to avoid breaking workloads
-          if (k.toLowerCase(Locale.ROOT).startsWith("delta.")) {
-            props.put(k, v)
-          }
-        }
+        val deltaPrefix: Map[String, String] = writeOptions.filter { case (k, _) =>
+          k.toLowerCase(Locale.ROOT).startsWith("delta.") }
+
+        catalogProp.properties ++ deltaPrefix


This should also be added to DeltaTableProperties. We would like to be consistent in all places.

…s as for the underlying data delta-io#809 Signed-off-by: Lucian Neghina <[email protected]>

zsxwing · 2021-12-20T07:59:15Z

Sorry for being late. Although the change looks good, I will need to take some time to think about the backward compatibility.

zsxwing · 2022-05-04T18:33:56Z

Sorry for the delay. I'm currently working on adding a new test suite to check the existing behaviors in all APIs that we can set options.

zsxwing · 2022-05-06T16:47:37Z

Just to give a update. @scottsand-db is working on adding tests for the following complicated test matrix to check what are the behaviors in all combinations (Unfortunately, we have too many ways to create/write a Delta table today and they are not sharing the same code path):

Two types of options:

Options starting with delta.
Options not starting with delta.

Two ways to refer a Delta table

Path
Table name

APIs to write a Delta table

DataFrameWriter save/saveAsTable
DataStreamWriter (streaming) start/table
Dataset.writeStream
DataFrameWriterV2
DeltaTable.create
DeltaTable.replace
DeltaTable.createOrReplace
SQL
- CREATE TABLE … USING DELTA TBLPROPERTIES(...)
- CREATE TABLE … USING DELTA OPTIONS(...)
- CREATE TABLE … USING DELTA OPTIONS(...) TBLPROPERTIES()
- REPLACE
- CREATE OR REPLACE
- CREATE TABLE … AS SELECT * FROM XXX
Different table states:
- Create a new table
- Overwrite an existing table
- Append to an existing table

We want to have these tests first so that we can make sure any new changes will be consistent in all of these cases.

And thanks a lot, @scottsand-db for offering the help to do this.

scottsand-db · 2022-05-09T21:32:02Z

Here is the test suite which shows all the different ways to write configs to a delta table.
#1120

scottsand-db · 2022-05-17T17:02:36Z

@nflucian just FYI we are still working on merging #1120 first before this PR.

scottsand-db · 2022-05-20T20:46:41Z

Hi @nflucian, #1120 has been merged. It adds a test suite that shows the status of all the different combinations of writing configs into delta.

Can you update your branch with master? Then, could you please run `build/sbt 'core/testOnly *DeltaWriteConfigsSuite' and double check that your changes solve the problem? Can you then please update the relevant comment tables (e.g. here) with the latest result.

Let me know if you have any questions?

scottsand-db

Left some comments. There's also 3 other big changes this PR needs.

We should only write options that are prefixed with delta. BUT we also need to be able to read old tables that do have options that aren't prefixed with delta. (Backwards compatibility)
We need a feature flag, which when enabled rolls back to the previous (i.e. current) behaviour.
A unit test that tests this backwards compatibility. See EvolvabilitySuite

scottsand-db · 2022-05-24T23:30:50Z

core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaTableProperties.scala

+    new DeltaTableProperties(tableProperties, writeOptions)
+  }
+
+  private def deltaProperties(writeOptions: Map[String, String]): Map[String, String] = {


scaladoc. describe the input and the output

scottsand-db · 2022-05-24T23:30:50Z

core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaTableProperties.scala

+    new DeltaTableProperties(tableProperties, writeOptions)
+  }
+
+  private def deltaProperties(writeOptions: Map[String, String]): Map[String, String] = {


scaladoc. describe the input and the output

scottsand-db · 2022-05-24T23:31:01Z

core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaTableProperties.scala

+    }
+  }
+
+  private def split(properties: Map[String, String]): (Map[String, String], Map[String, String]) = {


scaladoc: describe the output. what are the tuples?

scottsand-db · 2022-05-24T23:31:01Z

core/src/main/scala/org/apache/spark/sql/delta/catalog/DeltaTableProperties.scala

+    }
+  }
+
+  private def split(properties: Map[String, String]): (Map[String, String], Map[String, String]) = {


scaladoc: describe the output. what are the tuples?

scottsand-db · 2022-05-24T23:31:53Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaSuite.scala

+    withTempDir { tempDir =>
+      val path = tempDir.getCanonicalPath
+      val options = Map(
+        "header"-> "true",


After syncing with @zsxwing , we've confirmed that we do not want to write properties that are not prefixed with delta. Thus, to be clear, we only want to write to table metadata those properties which start with delta.

scottsand-db · 2022-05-24T23:31:53Z

core/src/test/scala/org/apache/spark/sql/delta/DeltaSuite.scala

+    withTempDir { tempDir =>
+      val path = tempDir.getCanonicalPath
+      val options = Map(
+        "header"-> "true",


After syncing with @zsxwing , we've confirmed that we do not want to write properties that are not prefixed with delta. Thus, to be clear, we only want to write to table metadata those properties which start with delta.

nflucian mentioned this pull request Nov 1, 2021

Creating an external table fails when it is given the same options as for the underlying data #809

Open

nflucian force-pushed the fix/create-table-options branch from f038c28 to 2de06d6 Compare November 1, 2021 19:56

dennyglee assigned allisonport-db Nov 3, 2021

dennyglee requested a review from zsxwing November 9, 2021 17:33

nflucian force-pushed the fix/create-table-options branch 4 times, most recently from d28b58b to 59b4293 Compare November 25, 2021 13:23

zsxwing requested changes Dec 2, 2021

View reviewed changes

Fix Creating an external table fails when it is given the same option…

76ed376

…s as for the underlying data delta-io#809 Signed-off-by: Lucian Neghina <[email protected]>

nflucian force-pushed the fix/create-table-options branch from 59b4293 to 76ed376 Compare December 7, 2021 11:32

nflucian requested a review from zsxwing December 7, 2021 14:10

Merge branch 'master' into fix/create-table-options

327fb1f

allisonport-db removed their assignment May 3, 2022

scottsand-db self-requested a review May 4, 2022 19:20

tdas removed the request for review from zsxwing May 6, 2022 19:16

scottsand-db added the enhancement New feature or request label May 20, 2022

Merge branch 'delta-io:master' into fix/create-table-options

7369eca

scottsand-db requested changes May 24, 2022

View reviewed changes

Issue #809: Fix Creating an external table options #820

Are you sure you want to change the base?

Issue #809: Fix Creating an external table options #820

Conversation

nflucian commented Nov 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

allisonport-db commented Nov 12, 2021

Uh oh!

zsxwing commented Nov 17, 2021

Uh oh!

zsxwing Dec 2, 2021

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Dec 20, 2021

Uh oh!

zsxwing commented May 4, 2022

Uh oh!

zsxwing commented May 6, 2022

Uh oh!

scottsand-db commented May 9, 2022

Uh oh!

scottsand-db commented May 17, 2022

Uh oh!

scottsand-db commented May 20, 2022

Uh oh!

scottsand-db left a comment

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

scottsand-db May 24, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nflucian commented Nov 1, 2021 •

edited

Loading