You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2025/04/10 00:15:13 UTC
Re: [PR] [SPARK-51726][SQL] Use TableInfo for Stage CREATE/REPLACE/CREATE OR REPLACE table [spark]
aokolnychyi commented on code in PR #50521:
URL: https://github.com/apache/spark/pull/50521#discussion_r2036308831
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/StagingTableCatalog.java:
##########
@@ -134,22 +161,16 @@ default StagedTable stageReplace(
* operation.
*
* @param ident a table identifier
- * @param columns the columns of the new table
- * @param partitions transforms to use for partitioning data in the table
- * @param properties a string map of table properties
+ * @param tableInfo information about the table
* @return metadata for the new table. This can be null if the catalog does not support atomic
* creation for this table. Spark will call {@link #loadTable(Identifier)} later.
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
* @throws NoSuchTableException If the table does not exist
*/
- default StagedTable stageReplace(
- Identifier ident,
- Column[] columns,
- Transform[] partitions,
- Map<String, String> properties) throws NoSuchNamespaceException, NoSuchTableException {
- return stageReplace(
- ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
+ default StagedTable stageReplace(Identifier ident, TableInfo tableInfo)
Review Comment:
Optional: Same comment about formatting as in create.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/StagingTableCatalog.java:
##########
@@ -186,21 +224,18 @@ default StagedTable stageCreateOrReplace(
* the staged changes are committed but the table doesn't exist at commit time.
*
* @param ident a table identifier
- * @param columns the columns of the new table
- * @param partitions transforms to use for partitioning data in the table
- * @param properties a string map of table properties
+ * @param tableInfo information about the table
* @return metadata for the new table. This can be null if the catalog does not support atomic
* creation for this table. Spark will call {@link #loadTable(Identifier)} later.
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
- default StagedTable stageCreateOrReplace(
- Identifier ident,
- Column[] columns,
- Transform[] partitions,
- Map<String, String> properties) throws NoSuchNamespaceException {
- return stageCreateOrReplace(
- ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
+ default StagedTable stageCreateOrReplace(Identifier ident, TableInfo tableInfo)
Review Comment:
Here too, method + invocation.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/StagingTableCatalog.java:
##########
@@ -115,6 +125,23 @@ default StagedTable stageReplace(
throw QueryCompilationErrors.mustOverrideOneMethodError("stageReplace");
}
+ /**
+ * Stage the replacement of a table, preparing it to be committed into the metastore when the
+ * returned table's {@link StagedTable#commitStagedChanges()} is called.
+ * <p>
+ * This is deprecated, please override
+ * {@link #stageReplace(Identifier, TableInfo)} instead.
+ */
+ @Deprecated(since = "4.1.0")
+ default StagedTable stageReplace(
+ Identifier ident,
+ Column[] columns,
+ Transform[] partitions,
+ Map<String, String> properties) throws NoSuchNamespaceException, NoSuchTableException {
+ return stageReplace(
+ ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
Review Comment:
Optional: What about putting each argument on a separate line?
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/StagingTableCatalog.java:
##########
@@ -82,21 +97,16 @@ default StagedTable stageCreate(
* committed, an exception should be thrown by {@link StagedTable#commitStagedChanges()}.
*
* @param ident a table identifier
- * @param columns the column of the new table
- * @param partitions transforms to use for partitioning data in the table
- * @param properties a string map of table properties
+ * @param tableInfo information about the table
* @return metadata for the new table. This can be null if the catalog does not support atomic
* creation for this table. Spark will call {@link #loadTable(Identifier)} later.
* @throws TableAlreadyExistsException If a table or view already exists for the identifier
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
- default StagedTable stageCreate(
- Identifier ident,
- Column[] columns,
- Transform[] partitions,
- Map<String, String> properties) throws TableAlreadyExistsException, NoSuchNamespaceException {
- return stageCreate(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
+ default StagedTable stageCreate(Identifier ident, TableInfo tableInfo)
Review Comment:
Optional: My personal preference would be to format it this way:
```
default StagedTable stageCreate(
Identifier ident,
TableInfo tableInfo) throws TableAlreadyExistsException, NoSuchNamespaceException {
...
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org