Skip to content

Commit

Permalink
Use name column mapping by default in Delta Lake
Browse files Browse the repository at this point in the history
  • Loading branch information
@ebyhr
ebyhr committed Jan 10, 2025
1 parent a6c0e52 commit b671917
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 98 deletions.
2 changes: 1 addition & 1 deletion docs/src/main/sphinx/connector/delta-lake.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ The following table properties are available for use:
* `NAME`
* `NONE`

Defaults to `NONE`.
Defaults to `NAME`.
* - `deletion_vectors_enabled`
- Enables deletion vectors.
:::
Expand Down
3 changes: 1 addition & 2 deletions ...in/trino-delta-lake/src/main/java/io/trino/plugin/deltalake/DeltaLakeTableProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ public DeltaLakeTableProperties(DeltaLakeConfig config)
.add(stringProperty(
COLUMN_MAPPING_MODE_PROPERTY,
"Column mapping mode. Possible values: [ID, NAME, NONE]",
// TODO: Consider using 'name' by default. 'none' column mapping doesn't support some statements
ColumnMappingMode.NONE.name(),
ColumnMappingMode.NAME.name(),
value -> {
EnumSet<ColumnMappingMode> allowed = EnumSet.of(ColumnMappingMode.ID, ColumnMappingMode.NAME, ColumnMappingMode.NONE);
if (allowed.stream().map(Enum::name).noneMatch(mode -> mode.equalsIgnoreCase(value))) {
Expand Down
128 changes: 33 additions & 95 deletions .../trino-delta-lake/src/test/java/io/trino/plugin/deltalake/TestDeltaLakeConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,7 @@ public void testShowCreateTable()
" comment varchar\n" +
")\n" +
"WITH (\n" +
" column_mapping_mode = 'NAME',\n" +
" location = \\E'.*/test_schema/orders.*'\n\\Q" +
")");
}
Expand Down Expand Up @@ -452,38 +453,6 @@ public void testDropNonEmptySchemaWithTable()
assertUpdate("DROP SCHEMA " + schemaName);
}

@Test
@Override
public void testDropColumn()
{
// Override because the connector doesn't support dropping columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(super::testDropColumn)
.hasMessageContaining("Cannot drop column from table using column mapping mode NONE");
}

@Test
@Override
public void testAddAndDropColumnName()
{
for (String columnName : testColumnNameDataProvider()) {
// Override because the connector doesn't support dropping columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(() -> testAddAndDropColumnName(columnName, requiresDelimiting(columnName)))
.hasMessageContaining("Cannot drop column from table using column mapping mode NONE");
}
}

@Test
@Override
public void testDropAndAddColumnWithSameName()
{
// Override because the connector doesn't support dropping columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(super::testDropAndAddColumnWithSameName)
.hasMessageContaining("Cannot drop column from table using column mapping mode NONE");
}

@Test
public void testDropPartitionColumn()
{
Expand Down Expand Up @@ -512,26 +481,6 @@ public void testDropLastNonPartitionColumn()
assertUpdate("DROP TABLE " + tableName);
}

@Test
@Override
public void testRenameColumn()
{
// Override because the connector doesn't support renaming columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(super::testRenameColumn)
.hasMessageContaining("Cannot rename column in table using column mapping mode NONE");
}

@Test
@Override
public void testRenameColumnWithComment()
{
// Override because the connector doesn't support renaming columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(super::testRenameColumnWithComment)
.hasMessageContaining("Cannot rename column in table using column mapping mode NONE");
}

@Test
public void testDeltaRenameColumnWithComment()
{
Expand Down Expand Up @@ -559,28 +508,6 @@ private void testDeltaRenameColumnWithComment(ColumnMappingMode mode)
assertUpdate("DROP TABLE " + tableName);
}

@Test
@Override
public void testAlterTableRenameColumnToLongName()
{
// Override because the connector doesn't support renaming columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(super::testAlterTableRenameColumnToLongName)
.hasMessageContaining("Cannot rename column in table using column mapping mode NONE");
}

@Test
@Override
public void testRenameColumnName()
{
for (String columnName : testColumnNameDataProvider()) {
// Override because the connector doesn't support renaming columns with 'none' column mapping
// There are some tests in in io.trino.tests.product.deltalake.TestDeltaLakeColumnMappingMode
assertThatThrownBy(() -> testRenameColumnName(columnName, requiresDelimiting(columnName)))
.hasMessageContaining("Cannot rename column in table using column mapping mode NONE");
}
}

@Test
@Override
public void testCharVarcharComparison()
Expand Down Expand Up @@ -1345,23 +1272,28 @@ public void testCreateTableWithChangeDataFeed()
assertThat(query("SELECT * FROM \"" + table.getName() + "$properties\""))
.skippingTypesCheck()
.matches("VALUES " +
"('delta.columnMapping.mode', 'name')," +
"('delta.columnMapping.maxColumnId', '1')," +
"('delta.enableChangeDataFeed', 'true')," +
"('delta.enableDeletionVectors', 'false')," +
"('delta.minReaderVersion', '1')," +
"('delta.minWriterVersion', '4')");
"('delta.minReaderVersion', '2')," +
"('delta.minWriterVersion', '5')");
}

// timestamp type requires reader version 3 and writer version 7
try (TestTable table = newTrinoTable("test_cdf", "(x timestamp) WITH (change_data_feed_enabled = true)")) {
assertThat(query("SELECT * FROM \"" + table.getName() + "$properties\""))
.skippingTypesCheck()
.matches("VALUES " +
"('delta.columnMapping.mode', 'name')," +
"('delta.columnMapping.maxColumnId', '1')," +
"('delta.enableChangeDataFeed', 'true')," +
"('delta.enableDeletionVectors', 'false')," +
"('delta.minReaderVersion', '3')," +
"('delta.minWriterVersion', '7')," +
"('delta.feature.timestampNtz', 'supported')," +
"('delta.feature.changeDataFeed', 'supported')");
"('delta.feature.changeDataFeed', 'supported')," +
"('delta.feature.columnMapping', 'supported')");
}
}

Expand Down Expand Up @@ -2044,6 +1976,7 @@ public void testCreateOrReplaceTableChangePartitionedTableIntoUnpartitioned()
" b varchar\n" +
"\\)\n" +
"WITH \\(\n" +
" column_mapping_mode = 'NAME',\n" +
" location = '.*'\n" +
"\\)");
}
Expand Down Expand Up @@ -2319,7 +2252,7 @@ public void testTableOperationWithChangeInColumnMappingMode(String columnMapping
{
try (TestTable table = newTrinoTable(
"create_or_replace_with_change_column_mapping_",
" AS SELECT 1 as colA, 'B' as colB")) {
"WITH (column_mapping_mode = 'NONE') AS SELECT 1 as colA, 'B' as colB")) {
assertQueryFails(
"ALTER TABLE " + table.getName() + " DROP COLUMN colA",
"Cannot drop column from table using column mapping mode NONE");
Expand Down Expand Up @@ -2764,7 +2697,7 @@ public void testProjectionPushdownColumnReorderInSchemaAndDataFile()
public void testProjectionPushdownExplain()
{
String tableName = "test_projection_pushdown_explain_" + randomNameSuffix();
assertUpdate("CREATE TABLE " + tableName + " (id BIGINT, root ROW(f1 BIGINT, f2 BIGINT)) WITH (partitioned_by = ARRAY['id'])");
assertUpdate("CREATE TABLE " + tableName + " (id BIGINT, root ROW(f1 BIGINT, f2 BIGINT)) WITH (partitioned_by = ARRAY['id'], column_mapping_mode = 'NONE')");

assertExplain(
"EXPLAIN SELECT root.f2 FROM " + tableName,
Expand All @@ -2790,7 +2723,7 @@ public void testProjectionPushdownNonPrimitiveTypeExplain()
String tableName = "test_projection_pushdown_non_primtive_type_" + randomNameSuffix();

assertUpdate("CREATE TABLE " + tableName +
" (id BIGINT, _row ROW(child BIGINT), _array ARRAY(ROW(child BIGINT)), _map MAP(BIGINT, BIGINT))");
" (id BIGINT, _row ROW(child BIGINT), _array ARRAY(ROW(child BIGINT)), _map MAP(BIGINT, BIGINT)) WITH (column_mapping_mode = 'NONE')");

assertExplain(
"EXPLAIN SELECT id, _row.child, _array[1].child, _map[1] FROM " + tableName,
Expand Down Expand Up @@ -4171,20 +4104,25 @@ void testAddTimestampNtzColumnToCdfEnabledTable()
try (TestTable table = new TestTable(getQueryRunner()::execute, "test_timestamp_ntz", "(x int) WITH (change_data_feed_enabled = true)")) {
assertThat(getTableProperties(table.getName()))
.containsExactlyInAnyOrderEntriesOf(ImmutableMap.<String, String>builder()
.put("delta.columnMapping.mode", "name")
.put("delta.columnMapping.maxColumnId", "1")
.put("delta.enableChangeDataFeed", "true")
.put("delta.enableDeletionVectors", "false")
.put("delta.minReaderVersion", "1")
.put("delta.minWriterVersion", "4")
.put("delta.minReaderVersion", "2")
.put("delta.minWriterVersion", "5")
.buildOrThrow());

assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN ts TIMESTAMP");

// CDF is enabled in this table. 'delta.feature.changeDataFeed' must be added when updating the table to versions supporting table features
assertThat(getTableProperties(table.getName()))
.containsExactlyInAnyOrderEntriesOf(ImmutableMap.<String, String>builder()
.put("delta.columnMapping.mode", "name")
.put("delta.columnMapping.maxColumnId", "2")
.put("delta.enableChangeDataFeed", "true")
.put("delta.enableDeletionVectors", "false")
.put("delta.feature.changeDataFeed", "supported")
.put("delta.feature.columnMapping", "supported")
.put("delta.feature.timestampNtz", "supported")
.put("delta.minReaderVersion", "3")
.put("delta.minWriterVersion", "7")
Expand Down Expand Up @@ -4942,7 +4880,7 @@ void testRegisterTableAccessControl()
@Test
public void testMetastoreAfterCreateTable()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment'")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment' WITH (column_mapping_mode = 'NONE')")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
entry("comment", "test comment"),
Expand All @@ -4954,8 +4892,8 @@ public void testMetastoreAfterCreateTable()
@Test
public void testMetastoreAfterCreateOrReplaceTable()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment'")) {
assertUpdate("CREATE OR REPLACE TABLE " + table.getName() + "(new_col varchar) COMMENT 'new comment'");
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment' WITH (column_mapping_mode = 'NONE')")) {
assertUpdate("CREATE OR REPLACE TABLE " + table.getName() + "(new_col varchar) COMMENT 'new comment' WITH (column_mapping_mode = 'NONE')");
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
entry("comment", "new comment"),
Expand All @@ -4967,7 +4905,7 @@ public void testMetastoreAfterCreateOrReplaceTable()
@Test
public void testMetastoreAfterCreateTableAsSelect()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "COMMENT 'test comment' AS SELECT 1 col")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "COMMENT 'test comment' WITH (column_mapping_mode = 'NONE') AS SELECT 1 col")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
entry("comment", "test comment"),
Expand All @@ -4980,7 +4918,7 @@ public void testMetastoreAfterCreateTableAsSelect()
public void testMetastoreAfterCreateOrReplaceTableAsSelect()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "COMMENT 'test comment' AS SELECT 1 col")) {
assertUpdate("CREATE OR REPLACE TABLE " + table.getName() + " COMMENT 'new comment' AS SELECT 'test' new_col", 1);
assertUpdate("CREATE OR REPLACE TABLE " + table.getName() + " COMMENT 'new comment' WITH (column_mapping_mode = 'NONE') AS SELECT 'test' new_col", 1);
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
entry("comment", "new comment"),
Expand All @@ -4992,7 +4930,7 @@ public void testMetastoreAfterCreateOrReplaceTableAsSelect()
@Test
public void testMetastoreAfterCommentTable()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int)")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) WITH (column_mapping_mode = 'NONE')")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.doesNotContainKey("comment")
.contains(
Expand All @@ -5011,7 +4949,7 @@ public void testMetastoreAfterCommentTable()
@Test
public void testMetastoreAfterCommentColumn()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int COMMENT 'test comment')")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int COMMENT 'test comment') WITH (column_mapping_mode = 'NONE')")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.doesNotContainKey("comment")
.contains(
Expand Down Expand Up @@ -5092,7 +5030,7 @@ public void testMetastoreAfterAlterColumn()
@Test
public void testMetastoreAfterSetTableProperties()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int)")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) WITH (column_mapping_mode = 'NONE')")) {
assertUpdate("ALTER TABLE " + table.getName() + " SET PROPERTIES change_data_feed_enabled = true");
assertEventually(() -> assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
Expand All @@ -5104,7 +5042,7 @@ public void testMetastoreAfterSetTableProperties()
@Test
public void testMetastoreAfterOptimize()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int)")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) WITH (column_mapping_mode = 'NONE')")) {
assertUpdate("ALTER TABLE " + table.getName() + " EXECUTE optimize");
assertEventually(() -> assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(
Expand All @@ -5116,7 +5054,7 @@ public void testMetastoreAfterOptimize()
@Test
public void testMetastoreAfterRegisterTable()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment'")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment' WITH (column_mapping_mode = 'NONE')")) {
assertUpdate("INSERT INTO " + table.getName() + " VALUES 1", 1);
String tableLocation = metastore.getTable(SCHEMA, table.getName()).orElseThrow().getStorage().getLocation();
metastore.dropTable(SCHEMA, table.getName(), false);
Expand All @@ -5133,7 +5071,7 @@ public void testMetastoreAfterRegisterTable()
@Test
public void testMetastoreAfterCreateTableRemotely()
{
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment'")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) COMMENT 'test comment' WITH (column_mapping_mode = 'NONE')")) {
Table metastoreTable = metastore.getTable(SCHEMA, table.getName()).orElseThrow();
metastore.dropTable(SCHEMA, table.getName(), false);

Expand Down Expand Up @@ -5161,7 +5099,7 @@ public void testMetastoreAfterDataManipulation()
{
String schemaString = "{\"type\":\"struct\",\"fields\":[{\"name\":\"col\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}}]}";

try (TestTable table = newTrinoTable("test_cache_metastore", "(col int)")) {
try (TestTable table = newTrinoTable("test_cache_metastore", "(col int) WITH (column_mapping_mode = 'NONE')")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(entry("trino_last_transaction_version", "0"), entry("trino_metadata_schema_string", schemaString));

Expand Down Expand Up @@ -5195,7 +5133,7 @@ public void testMetastoreAfterTruncateTable()
{
String schemaString = "{\"type\":\"struct\",\"fields\":[{\"name\":\"col\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{}}]}";

try (TestTable table = newTrinoTable("test_cache_metastore", "AS SELECT 1 col")) {
try (TestTable table = newTrinoTable("test_cache_metastore", " WITH (column_mapping_mode = 'NONE') AS SELECT 1 col")) {
assertThat(metastore.getTable(SCHEMA, table.getName()).orElseThrow().getParameters())
.contains(entry("trino_last_transaction_version", "0"), entry("trino_metadata_schema_string", schemaString));

Expand Down

0 comments on commit b671917

Please sign in to comment.