Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Kernel] Allow setting arbitrary properties when creating/updating the table #4107

Merged
merged 4 commits into from
Jan 31, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,23 @@ public static Map<String, String> validateProperties(Map<String, String> configu
for (Map.Entry<String, String> 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) {
scottsand-db marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
Loading