Skip to content

Commit

Permalink
Revert "Allow adding and droping fields to records wrapped in array"
Browse files Browse the repository at this point in the history 
This change introduces behavior incompatible with #16959
  • Loading branch information
@martint
martint committed May 31, 2024
1 parent 7239574 commit 2ffc951
Show file tree
Hide file tree
Showing 6 changed files with 20 additions and 138 deletions.
7 changes: 1 addition & 6 deletions core/trino-main/src/main/java/io/trino/execution/AddColumnTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import io.trino.spi.connector.CatalogHandle;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.type.ArrayType;
import io.trino.spi.type.RowType;
import io.trino.spi.type.Type;
import io.trino.spi.type.TypeNotFoundException;
Expand Down Expand Up @@ -195,11 +194,7 @@ public ListenableFuture<Void> execute(

private static List<RowType.Field> getCandidates(Type type, String fieldName)
{
Type analyzedType = type;
if (type instanceof ArrayType arrayType) {
analyzedType = arrayType.getElementType();
}
if (!(analyzedType instanceof RowType rowType)) {
if (!(type instanceof RowType rowType)) {
throw new TrinoException(NOT_SUPPORTED, "Unsupported type: " + type);
}
List<RowType.Field> candidates = rowType.getFields().stream()
Expand Down
12 changes: 2 additions & 10 deletions core/trino-main/src/main/java/io/trino/execution/DropColumnTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,12 @@
import io.trino.security.AccessControl;
import io.trino.spi.connector.ColumnHandle;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.type.ArrayType;
import io.trino.spi.type.RowType;
import io.trino.spi.type.Type;
import io.trino.sql.tree.DropColumn;
import io.trino.sql.tree.Expression;
import io.trino.sql.tree.Identifier;

import java.util.Iterator;
import java.util.List;

import static com.google.common.base.Verify.verifyNotNull;
Expand Down Expand Up @@ -114,13 +112,7 @@ public ListenableFuture<Void> execute(
else {
RowType containingType = null;
Type currentType = columnMetadata.getType();
Iterator<String> fieldIterator = fieldPath.iterator();
while (fieldIterator.hasNext()) {
if (currentType instanceof ArrayType arrayType) {
currentType = arrayType.getElementType();
continue;
}
String fieldName = fieldIterator.next();
for (String fieldName : fieldPath) {
if (currentType instanceof RowType rowType) {
List<RowType.Field> candidates = rowType.getFields().stream()
// case-sensitive match
Expand All @@ -140,7 +132,7 @@ public ListenableFuture<Void> execute(
return immediateVoidFuture();
}
}
// TODO: Support map types
// TODO: Support array and map types
throw semanticException(
NOT_SUPPORTED,
statement,
Expand Down
16 changes: 16 additions & 0 deletions core/trino-main/src/test/java/io/trino/execution/TestAddColumnTask.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import io.trino.security.AllowAllAccessControl;
import io.trino.spi.connector.ColumnMetadata;
import io.trino.spi.connector.ConnectorTableMetadata;
import io.trino.spi.type.ArrayType;
import io.trino.spi.type.MapType;
import io.trino.spi.type.RowType;
import io.trino.spi.type.Type;
Expand Down Expand Up @@ -199,6 +200,21 @@ public void testAddFieldToNotExistingField()
.hasMessage("Field 'x' does not exist within row(a row(b integer))");
}

@Test
public void testUnsupportedArrayTypeInRowField()
{
QualifiedObjectName tableName = qualifiedObjectName("existing_table");
metadata.createTable(
testSession,
TEST_CATALOG_NAME,
rowTable(tableName, new RowType.Field(Optional.of("a"), new ArrayType(rowType(new RowType.Field(Optional.of("element"), INTEGER))))),
FAIL);

assertTrinoExceptionThrownBy(() -> getFutureValue(executeAddColumn(asQualifiedName(tableName), QualifiedName.of("col", "a", "c"), INTEGER, false, false)))
.hasErrorCode(NOT_SUPPORTED)
.hasMessage("Unsupported type: array(row(element integer))");
}

@Test
public void testUnsupportedMapTypeInRowField()
{
Expand Down
13 changes: 1 addition & 12 deletions plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1939,18 +1939,7 @@ public void addField(ConnectorSession session, ConnectorTableHandle tableHandle,
NestedField parent = icebergTable.schema().caseInsensitiveFindField(parentName);

String caseSensitiveParentName = icebergTable.schema().findColumnName(parent.fieldId());

Types.StructType structType;
if (parent.type().isListType()) {
// list(struct...)
structType = parent.type().asListType().elementType().asStructType();
}
else {
// just struct
structType = parent.type().asStructType();
}

NestedField field = structType.caseInsensitiveField(fieldName);
NestedField field = parent.type().asStructType().caseInsensitiveField(fieldName);
if (field != null) {
if (ignoreExisting) {
return;
Expand Down
108 changes: 0 additions & 108 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,6 @@
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_NOT_NULL_CONSTRAINT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_COLUMN_WITH_COMMENT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_FIELD;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ADD_FIELD_IN_ARRAY;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_ARRAY;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_COLUMN;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_COMMENT_ON_MATERIALIZED_VIEW_COLUMN;
Expand All @@ -127,7 +126,6 @@
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DEREFERENCE_PUSHDOWN;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DROP_COLUMN;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DROP_FIELD;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DROP_FIELD_IN_ARRAY;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DROP_NOT_NULL_CONSTRAINT;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_DROP_SCHEMA_CASCADE;
import static io.trino.testing.TestingConnectorBehavior.SUPPORTS_INSERT;
Expand Down Expand Up @@ -2435,48 +2433,6 @@ public void testAddRowField()
}
}

@Test
public void testAddRowFieldInArray()
{
skipTestUnless(hasBehavior(SUPPORTS_CREATE_TABLE_WITH_DATA) && hasBehavior(SUPPORTS_ROW_TYPE));

if (!hasBehavior(SUPPORTS_ADD_FIELD_IN_ARRAY)) {
try (TestTable table = new TestTable(getQueryRunner()::execute, "test_add_field_in_array_", "AS SELECT CAST(array[row(1)] AS array(row(x integer))) AS col")) {
assertQueryFails(
"ALTER TABLE " + table.getName() + " ADD COLUMN col.y integer",
".*does not support.*");
}
return;
}

try (TestTable table = new TestTable(getQueryRunner()::execute,
"test_add_field_in_array_",
"AS SELECT CAST(array[row(1, row(10), array[row(11)])] AS array(row(a integer, b row(x integer), c array(row(v integer))))) AS col")) {
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b row(x integer), c array(row(v integer))))");

assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.d integer");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b row(x integer), c array(row(v integer)), d integer))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10), array[row(11)], NULL)] AS array(row(a integer, b row(x integer), c array(row(v integer)), d integer)))");

// Add a nested field
assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.b.y integer");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b row(x integer, y integer), c array(row(v integer)), d integer))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10, NULL), array[row(11)], NULL)] AS array(row(a integer, b row(x integer, y integer), c array(row(v integer)), d integer)))");

assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN col.c.w integer");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b row(x integer, y integer), c array(row(v integer, w integer)), d integer))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10, NULL), array[row(11, NULL)], NULL)] AS array(row(a integer, b row(x integer, y integer), c array(row(v integer, w integer)), d integer)))");

// Specify existing fields with IF NOT EXISTS option
assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN IF NOT EXISTS col.a varchar");
assertUpdate("ALTER TABLE " + table.getName() + " ADD COLUMN IF NOT EXISTS col.b.x varchar");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b row(x integer, y integer), c array(row(v integer, w integer)), d integer))");

// Specify existing fields without IF NOT EXISTS option
assertQueryFails("ALTER TABLE " + table.getName() + " ADD COLUMN col.a varchar", ".* Field 'a' already exists");
}
}

@Test
public void testDropColumn()
{
Expand Down Expand Up @@ -2554,70 +2510,6 @@ public void testDropRowField()
}
}

@Test
public void testDropRowFieldInArray()
{
if (!hasBehavior(SUPPORTS_DROP_FIELD_IN_ARRAY)) {
if (!hasBehavior(SUPPORTS_DROP_COLUMN) || !hasBehavior(SUPPORTS_ROW_TYPE)) {
return;
}
try (TestTable table = new TestTable(getQueryRunner()::execute, "test_drop_field_in_array_", "AS SELECT CAST(array[row(1, 2)] AS array(row(x integer, y integer))) AS col")) {
assertQueryFails(
"ALTER TABLE " + table.getName() + " DROP COLUMN col.x",
".*does not support.*");
}
return;
}

try (TestTable table = new TestTable(getQueryRunner()::execute,
"test_drop_field_in_array_",
"AS SELECT CAST(array[row(1, 2, row(10, 20), array[row(30, 40)])] AS array(row(a integer, b integer, c row(x integer, y integer), d array(row(v integer, w integer))))) AS col")) {
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, b integer, c row(x integer, y integer), d array(row(v integer, w integer))))");

assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN col.b");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, c row(x integer, y integer), d array(row(v integer, w integer))))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10, 20), array[row(30, 40)])] AS array(row(a integer, c row(x integer, y integer), d array(row(v integer, w integer)))))");

// Drop a nested field
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN col.c.y");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, c row(x integer), d array(row(v integer, w integer))))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10), array[row(30, 40)])] AS array(row(a integer, c row(x integer), d array(row(v integer, w integer)))))");

// Drop a nested field in array
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN col.d.v");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, c row(x integer), d array(row(w integer))))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, row(10), array[row(40)])] AS array(row(a integer, c row(x integer), d array(row(w integer)))))");

// Verify failure when trying to drop unique field in nested row type
assertQueryFails("ALTER TABLE " + table.getName() + " DROP COLUMN col.c.x", ".* Cannot drop the only field in a row type");
assertQueryFails("ALTER TABLE " + table.getName() + " DROP COLUMN col.d.w", ".* Cannot drop the only field in a row type");

// Verify failure when trying to drop non-existing fields
assertQueryFails(
"ALTER TABLE " + table.getName() + " DROP COLUMN col.c.non_existing",
"\\Qline 1:1: Cannot resolve field 'non_existing' within row(x integer) type when dropping [c, non_existing] in array(row(a integer, c row(x integer), d array(row(w integer))))");

// Verify failure when trying to drop non-existing fields
assertQueryFails(
"ALTER TABLE " + table.getName() + " DROP COLUMN col.d.non_existing",
"\\Qline 1:1: Cannot resolve field 'non_existing' within row(w integer) type when dropping [d, non_existing] in array(row(a integer, c row(x integer), d array(row(w integer))))");

// Drop a row having fields
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN col.c");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer, d array(row(w integer))))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1, array[row(40)])] AS array(row(a integer, d array(row(w integer)))))");

// Drop a array(row) having fields
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN col.d");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer))");
assertThat(query("SELECT * FROM " + table.getName())).matches("SELECT CAST(array[row(1)] AS array(row(a integer)))");

// Specify non-existing fields with IF EXISTS option
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN IF EXISTS non_existing.a");
assertUpdate("ALTER TABLE " + table.getName() + " DROP COLUMN IF EXISTS col.non_existing");
assertThat(getColumnType(table.getName(), "col")).isEqualTo("array(row(a integer))");
}
}
@Test
public void testDropRowFieldWhenDuplicates()
{
Expand Down
2 changes: 0 additions & 2 deletions testing/trino-testing/src/main/java/io/trino/testing/TestingConnectorBehavior.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,8 @@ public enum TestingConnectorBehavior
SUPPORTS_ADD_COLUMN,
SUPPORTS_ADD_COLUMN_WITH_COMMENT(SUPPORTS_ADD_COLUMN),
SUPPORTS_ADD_FIELD(fallback -> fallback.test(SUPPORTS_ADD_COLUMN) && fallback.test(SUPPORTS_ROW_TYPE)),
SUPPORTS_ADD_FIELD_IN_ARRAY(SUPPORTS_ADD_FIELD),
SUPPORTS_DROP_COLUMN(SUPPORTS_ADD_COLUMN),
SUPPORTS_DROP_FIELD(and(SUPPORTS_DROP_COLUMN, SUPPORTS_ROW_TYPE)),
SUPPORTS_DROP_FIELD_IN_ARRAY(SUPPORTS_ADD_FIELD_IN_ARRAY),
SUPPORTS_RENAME_COLUMN,
SUPPORTS_RENAME_FIELD(fallback -> fallback.test(SUPPORTS_RENAME_COLUMN) && fallback.test(SUPPORTS_ROW_TYPE)),
SUPPORTS_SET_COLUMN_TYPE,
Expand Down

0 comments on commit 2ffc951

Please sign in to comment.