From 5818a6bdb5e8f06029433ab4935b70eeb24b22a5 Mon Sep 17 00:00:00 2001 From: Venki Korukanti Date: Thu, 30 Jan 2025 13:38:19 -0800 Subject: [PATCH] [Kernel] Allow setting arbitary properties when creating/updating the table --- .../io/delta/kernel/internal/TableConfig.java | 19 ++-- .../defaults/DeltaTableWritesSuite.scala | 23 ----- .../defaults/TablePropertiesSutie.scala | 91 +++++++++++++++++++ 3 files changed, 104 insertions(+), 29 deletions(-) create mode 100644 kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TablePropertiesSutie.scala diff --git a/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableConfig.java b/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableConfig.java index aa82b284560..3399b572633 100644 --- a/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableConfig.java +++ b/kernel/kernel-api/src/main/java/io/delta/kernel/internal/TableConfig.java @@ -204,16 +204,23 @@ public static Map validateProperties(Map configu for (Map.Entry kv : configurations.entrySet()) { String key = kv.getKey().toLowerCase(Locale.ROOT); String value = kv.getValue(); - if (key.startsWith("delta.") && VALID_PROPERTIES.containsKey(key)) { + + if (key.startsWith("delta.")) { + // If it is a delta table property, make sure it is a supported property and editable + if (!VALID_PROPERTIES.containsKey(key)) { + throw DeltaErrors.unknownConfigurationException(kv.getKey()); + } + TableConfig tableConfig = VALID_PROPERTIES.get(key); - if (tableConfig.editable) { - tableConfig.validate(value); - validatedConfigurations.put(tableConfig.getKey(), value); - } else { + if (!tableConfig.editable) { throw DeltaErrors.cannotModifyTableProperty(kv.getKey()); } + + tableConfig.validate(value); + validatedConfigurations.put(tableConfig.getKey(), value); } else { - throw DeltaErrors.unknownConfigurationException(kv.getKey()); + // allow unknown properties to be set + validatedConfigurations.put(key, value); } } return validatedConfigurations; diff --git a/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala b/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala index 5907fb32108..83b77ef208b 100644 --- a/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala +++ b/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/DeltaTableWritesSuite.scala @@ -292,29 +292,6 @@ class DeltaTableWritesSuite extends DeltaTableWriteSuiteBase with ParquetSuiteBa } } - test("create table - invalid properties - expect failure") { - withTempDirAndEngine { (tablePath, engine) => - val ex1 = intercept[UnknownConfigurationException] { - createTxn( - engine, tablePath, isNewTable = true, testSchema, Seq.empty, Map("invalid key" -> "10")) - } - assert(ex1.getMessage.contains("Unknown configuration was specified: invalid key")) - - val ex2 = intercept[InvalidConfigurationValueException] { - createTxn( - engine, - tablePath, - isNewTable = true, - testSchema, Seq.empty, Map(TableConfig.CHECKPOINT_INTERVAL.getKey -> "-1")) - } - assert( - ex2.getMessage.contains( - String.format( - "Invalid value for table property '%s': '%s'. %s", - TableConfig.CHECKPOINT_INTERVAL.getKey, "-1", "needs to be a positive integer."))) - } - } - test("create partitioned table - partition column is not part of the schema") { withTempDirAndEngine { (tablePath, engine) => val table = Table.forPath(engine, tablePath) diff --git a/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TablePropertiesSutie.scala b/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TablePropertiesSutie.scala new file mode 100644 index 00000000000..ed4be80a8f7 --- /dev/null +++ b/kernel/kernel-defaults/src/test/scala/io/delta/kernel/defaults/TablePropertiesSutie.scala @@ -0,0 +1,91 @@ +/* + * Copyright (2021) The Delta Lake Project Authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.delta.kernel.defaults + +import io.delta.kernel.Table +import io.delta.kernel.exceptions.UnknownConfigurationException +import io.delta.kernel.internal.SnapshotImpl +import io.delta.kernel.utils.CloseableIterable.emptyIterable + +import scala.collection.immutable.Seq + +/** + * Suite to set or get table properties. + * TODO: for now we just have the support for `set`. API `get` will be added in the next PRs. + */ +class TablePropertiesSutie extends DeltaTableWriteSuiteBase { + test("create/update table - allow arbitrary properties") { + withTempDir { tempFile => + val tablePath = tempFile.getAbsolutePath + + // create table with arbitrary properties and check if they are set + createUpdateTableWithProps( + tablePath, + createTable = true, + props = Map("my key" -> "10", "my key2" -> "20") + ) + assertHasProp(tablePath, expProps = Map("my key" -> "10", "my key2" -> "20")) + + // update table by modifying the arbitrary properties and check if they are updated + createUpdateTableWithProps(tablePath, props = Map("my key" -> "30")) + assertHasProp(tablePath, expProps = Map("my key" -> "30", "my key2" -> "20")) + + // update table without any new properties and check if the existing properties are retained + createUpdateTableWithProps(tablePath) + assertHasProp(tablePath, expProps = Map("my key" -> "30", "my key2" -> "20")) + + // update table by adding new arbitrary properties and check if they are set + createUpdateTableWithProps(tablePath, props = Map("new key3" -> "str")) + assertHasProp( + tablePath, + expProps = Map("my key" -> "30", "my key2" -> "20", "new key3" -> "str")) + } + } + + test("create/update table - disallow unknown delta.* properties to Kernel") { + withTempDir { tempFile => + val tablePath = tempFile.getAbsolutePath + val ex1 = intercept[UnknownConfigurationException] { + createUpdateTableWithProps(tablePath, createTable = true, Map("delta.unknown" -> "str")) + } + assert(ex1.getMessage.contains("Unknown configuration was specified: delta.unknown")) + + // Try updating in an existing table + createUpdateTableWithProps(tablePath, createTable = true) + val ex2 = intercept[UnknownConfigurationException] { + createUpdateTableWithProps(tablePath, props = Map("Delta.unknown" -> "str")) + } + assert(ex2.getMessage.contains("Unknown configuration was specified: Delta.unknown")) + } + } + + def createUpdateTableWithProps( + tablePath: String, + createTable: Boolean = false, + props: Map[String, String] = null): Unit = { + createTxn(defaultEngine, tablePath, createTable, testSchema, Seq.empty, props) + .commit(defaultEngine, emptyIterable()) + } + + // TODO: this will be replaced with get API in the next PRs. + def assertHasProp(tablePath: String, expProps: Map[String, String]): Unit = { + val snapshot = Table.forPath(defaultEngine, tablePath) + .getLatestSnapshot(defaultEngine).asInstanceOf[SnapshotImpl] + expProps.foreach { case (key, value) => + assert(snapshot.getMetadata.getConfiguration.get(key) === value, key) + } + } +}