Copilot commented on code in PR #10757:
URL: https://github.com/apache/gravitino/pull/10757#discussion_r3114625433
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java:
##########
@@ -220,6 +221,46 @@ public void createTable(
catalogConnectorMetadata.createTable(table, saveMode == SaveMode.IGNORE);
}
+ @Override
+ public ConnectorOutputTableHandle beginCreateTable(
+ ConnectorSession session,
+ ConnectorTableMetadata tableMetadata,
+ Optional<ConnectorTableLayout> layout,
+ RetryMode retryMode,
+ boolean noExistingData) {
+ // First, create the table in the Gravitino catalog
+ GravitinoTable table = metadataAdapter.createTable(tableMetadata);
+ catalogConnectorMetadata.createTable(table, false);
+
+ // Get the table handle from the internal connector for the newly created
table
+ SchemaTableName tableName = tableMetadata.getTable();
+ ConnectorTableHandle internalTableHandle =
+ internalMetadata.getTableHandle(session, tableName, Optional.empty(),
Optional.empty());
+
+ // Delegate to the internal connector's insert path to write data,
+ // avoiding double table creation in the original connector
+ List<ColumnHandle> columns =
+ new ArrayList<>(internalMetadata.getColumnHandles(session,
internalTableHandle).values());
+ ConnectorInsertTableHandle insertTableHandle =
+ internalMetadata.beginInsert(session, internalTableHandle, columns,
retryMode);
+ return new GravitinoOutputTableHandle(insertTableHandle, tableName);
Review Comment:
`beginCreateTable()` creates the table in Gravitino before obtaining an
internal table handle / starting the insert. If
`internalMetadata.getTableHandle(...)` returns null or `beginInsert(...)`
throws, the method will fail after the table is already registered, leaving an
orphaned table (the failure mode this PR is trying to eliminate). Consider
wrapping the internal-handle lookup + `beginInsert` in a try/catch and dropping
the newly created table on failure; also handle the `internalTableHandle ==
null` case explicitly with a TrinoException.
```suggestion
SchemaTableName tableName = tableMetadata.getTable();
try {
// Get the table handle from the internal connector for the newly
created table
ConnectorTableHandle internalTableHandle =
internalMetadata.getTableHandle(session, tableName,
Optional.empty(), Optional.empty());
if (internalTableHandle == null) {
throw new TrinoException(
GRAVITINO_TABLE_NOT_EXISTS,
String.format("Table '%s' was created but is not available for
insert", tableName));
}
// Delegate to the internal connector's insert path to write data,
// avoiding double table creation in the original connector
List<ColumnHandle> columns =
new ArrayList<>(internalMetadata.getColumnHandles(session,
internalTableHandle).values());
ConnectorInsertTableHandle insertTableHandle =
internalMetadata.beginInsert(session, internalTableHandle,
columns, retryMode);
return new GravitinoOutputTableHandle(insertTableHandle, tableName);
} catch (RuntimeException e) {
try {
catalogConnectorMetadata.dropTable(table);
} catch (RuntimeException cleanupException) {
e.addSuppressed(cleanupException);
}
throw e;
}
```
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoOutputTableHandle.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.gravitino.trino.connector;
+
+import com.fasterxml.jackson.annotation.JsonCreator;
+import com.fasterxml.jackson.annotation.JsonProperty;
+import io.trino.spi.connector.ConnectorInsertTableHandle;
+import io.trino.spi.connector.ConnectorOutputTableHandle;
+import io.trino.spi.connector.SchemaTableName;
+
+/**
+ * The GravitinoOutputTableHandle is used for handling CTAS (CREATE TABLE AS
SELECT) operations.
+ *
+ * <p>Internally wraps a {@link ConnectorInsertTableHandle} because the
Gravitino connector creates
+ * the table first via catalogConnectorMetadata, then delegates data writing
to the internal
+ * connector's insert path, avoiding double table creation.
+ *
+ * <p>Also stores the {@link SchemaTableName} so that {@code
rollbackCreateTable} can drop the table
+ * from the Gravitino catalog if the CTAS operation fails.
+ */
Review Comment:
This JavaDoc states the handle is used by `rollbackCreateTable` to drop the
table on CTAS failure, but there is no `rollbackCreateTable` implementation in
the connector metadata shown in this PR. Either implement
`ConnectorMetadata.rollbackCreateTable(...)` to use `toSchemaTableName()` (and
ensure it’s invoked across supported SPI versions), or update/remove this
JavaDoc to avoid implying rollback semantics that aren’t actually provided.
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/GravitinoMetadata.java:
##########
@@ -220,6 +221,46 @@ public void createTable(
catalogConnectorMetadata.createTable(table, saveMode == SaveMode.IGNORE);
}
+ @Override
+ public ConnectorOutputTableHandle beginCreateTable(
+ ConnectorSession session,
+ ConnectorTableMetadata tableMetadata,
+ Optional<ConnectorTableLayout> layout,
+ RetryMode retryMode,
+ boolean noExistingData) {
+ // First, create the table in the Gravitino catalog
+ GravitinoTable table = metadataAdapter.createTable(tableMetadata);
+ catalogConnectorMetadata.createTable(table, false);
+
+ // Get the table handle from the internal connector for the newly created
table
+ SchemaTableName tableName = tableMetadata.getTable();
+ ConnectorTableHandle internalTableHandle =
+ internalMetadata.getTableHandle(session, tableName, Optional.empty(),
Optional.empty());
+
+ // Delegate to the internal connector's insert path to write data,
+ // avoiding double table creation in the original connector
+ List<ColumnHandle> columns =
+ new ArrayList<>(internalMetadata.getColumnHandles(session,
internalTableHandle).values());
Review Comment:
The `columns` list passed to `internalMetadata.beginInsert(...)` is built
from `getColumnHandles(...).values()`. Map value iteration order is not
guaranteed, and `beginInsert` expects the column handles to correspond to the
table columns in a deterministic order. Build the list in the same order as
`tableMetadata.getColumns()` (resolve each name to a handle) to avoid incorrect
column-to-value mapping during CTAS writes.
```suggestion
Map<String, ColumnHandle> internalColumnHandles =
internalMetadata.getColumnHandles(session, internalTableHandle);
List<ColumnHandle> columns = new
ArrayList<>(tableMetadata.getColumns().size());
for (ColumnMetadata columnMetadata : tableMetadata.getColumns()) {
ColumnHandle columnHandle =
internalColumnHandles.get(columnMetadata.getName());
if (columnHandle == null) {
throw new TrinoException(
GRAVITINO_COLUMN_NOT_EXISTS,
"Column does not exist: " + columnMetadata.getName());
}
columns.add(columnHandle);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
