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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
