You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "anoopj (via GitHub)" <gi...@apache.org> on 2025/03/03 19:29:40 UTC
[PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
anoopj opened a new pull request, #50137:
URL: https://github.com/apache/spark/pull/50137
### What changes were proposed in this pull request?
Introduces a builder pattern for `TableCatalog`. A followup PR will be sent for the table replace methods in `StagingTableCatalog`.
### Why are the changes needed?
The current `TableCatalog` interface has overloaded `createTable()` implementations. As we are adding new parameters, we have to deprecate the old ones. This PR introduces new builder pattern so that the code is cleaner.
### Does this PR introduce _any_ user-facing change?
### How was this patch tested?
* New unit tests.
* Existing tests in `DataSourceV2Suite`.
### Was this patch authored or co-authored using generative AI tooling?
No
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978922356
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
Review Comment:
Should we use null as the default value?
`private Transform[] partitions;`
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978551802
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
Review Comment:
to make it clear that it build table metadata, not table data, shall we name it `TableInfoBuilder`?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006505090
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
Review Comment:
Optional: I personally not a big fan of `getXXX` prefixes as they have no meaning. But that's usually something decided in each project. I wonder what @cloud-fan thinks here.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006508994
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -222,6 +226,7 @@ default Table createTable(
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
+ @Deprecated(since = "4.0.0")
Review Comment:
I agree, it will be shipped in 4.1, so it can't be deprecated since 4.0.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006620680
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
Review Comment:
Done
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
Review Comment:
Done
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1999997357
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,15 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
I guess we need a `buildTable` only with Identifier.
`default TableBuilder buildTable(Identifier ident)`
Do we need the `withColumns(...)` for `TableBuilder` ?
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978151420
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -294,6 +295,51 @@ class InMemoryTableCatalog extends BasicInMemoryTableCatalog with SupportsNamesp
}
case class Result(readSchema: StructType, rows: Array[InternalRow]) extends LocalScan
+
+ override def buildTable(ident: Identifier, columns: Array[Column]): TableBuilder = {
+ new BasicInMemoryTableBuilder(ident, columns, namespaces, tables)
+ }
+
+ private class BasicInMemoryTableBuilder(
+ val ident: Identifier,
+ val columns: Array[Column],
+ val namespaces: util.Map[List[String], Map[String, String]],
+ val tables: util.Map[Identifier, Table])
Review Comment:
I think we dont need this, as we can access outer class?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980111091
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.columns = columns;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withProperties(Map<String, String> properties) {
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ @Override
+ public Table create() throws TableAlreadyExistsException, NoSuchNamespaceException {
+ return catalog.createTable(identifier, columns, partitions, properties);
Review Comment:
I believe this class is a built-in implementation for backward compatibility. Connectors would implement their own builders.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980084846
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
I am inclined towards just making the API not additive. Most call sites already have the table properties already materialized as a map. Updating the PR and the Javadoc accordingly. Please let me know if that sounds reasonable.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1994546972
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.columns = columns;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withProperties(Map<String, String> properties) {
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ @Override
+ public Table create() throws TableAlreadyExistsException, NoSuchNamespaceException {
+ return catalog.createTable(identifier, columns, partitions, properties);
Review Comment:
I have a question. How to increase flexibility if users customize the `TableBuilder` and override the `create`.
It seems there is no flexible way.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009946968
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
Review Comment:
let's also add `StructType schema` to be fully on par with the previous `createTable` overloads: The implementation can choose to still process `StructType` instead of the new `Column`s.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009941051
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -230,6 +230,23 @@ default Table createTable(
return createTable(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
}
+ /**
+ * Create a table in the catalog.
+ *
+ * @param ident a table identifier
+ * @param tableInfo information about the table.
+ * @return metadata for the new table. This can be null if getting the metadata for the new table
+ * is expensive. Spark will call {@link #loadTable(Identifier)} if needed (e.g. CTAS).
+ *
+ * @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)
+ */
Review Comment:
let's add `@since 4.1.0`
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005729143
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -75,7 +75,10 @@ class CatalogSuite extends SparkFunSuite {
intercept[NoSuchNamespaceException](catalog.listTables(Array("ns")))
- catalog.createTable(ident1, columns, emptyTrans, emptyProps)
+ val tableInfo = new TableInfo.Builder(ident1, columns)
+ .withPartitions(emptyTrans)
Review Comment:
if we don't call `withPartitions`, it means empty partitions?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006508242
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -33,11 +33,11 @@
/**
* Catalog methods for working with Tables.
* <p>
- * TableCatalog implementations may be case sensitive or case insensitive. Spark will pass
+ * TableCatalog implementations may be case-sensitive or case-insensitive. Spark will pass
Review Comment:
These seem cosmetic and unrelated. I'd probably not mix these changes with adding `TableInfo`.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2703005083
From a user's point of view, I feel the new API is a bit cumbersome as I need to implement my own `TableBuilder`, and then implement `buildTable` to return it. `TableBuilder` has quite a few methods to implement and previously I just needed to implement a simple `createTable` method.
Thinking about it again, we want to make the API easier to implement, but not easier to invoke (only invoked by Spark internally). So the builder pattern doesn't seem to be a good choice here. I think we just need to have an interface to hold all table information, and let createTable/replaceTable take it instead of many parameters. For example:
```
interface TableInfo {
Column[] columns;
Transform[] partitions;
...
}
interface TableCatalog ... {
Table createTable(Identifier ident, TableInfo t);
}
```
Users just need to implement one method, and we can keep adding new fields in `TableInfo`, which is backward compatible as users only need to call its methods, but no need to implement it.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2707494998
@cloud-fan, I don't have a strong opinion on this one and can go either way.
One point to keep in mind that Spark will not have the guarantee that the newly added information to `TableChange` is going to be respected by the connector. For instance, if we add a new field to `TableInfo`, we won't know if the new field is going to be respected/supported or simply ignored. That said, we can mitigate that via capabilities. I do get the point of not implementing a builder in the connector.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006507093
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -758,6 +950,7 @@ class CatalogSuite extends SparkFunSuite {
val ident2 = Identifier.of(Array("ns1", "ns2"), "test_table_2")
catalog.createNamespace(Array("ns1"), Map("property" -> "value").asJava)
+
Review Comment:
Necessary?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006507093
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -758,6 +950,7 @@ class CatalogSuite extends SparkFunSuite {
val ident2 = Identifier.of(Array("ns1", "ns2"), "test_table_2")
catalog.createNamespace(Array("ns1"), Map("property" -> "value").asJava)
+
Review Comment:
Intentional?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006611828
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
Review Comment:
Looking at v2 `Expression`, we don't follow the Java tradition to name these getter methods. There is no `get` perfix. Let's do the same here.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
Review Comment:
Looking at v2 `Expression`, we don't follow the Java tradition to name these getter methods. There is no `get` prefix. Let's do the same here.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006615201
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -210,6 +210,10 @@ default Table createTable(
/**
* Create a table in the catalog.
+ * <p>
+ * @deprecated This is deprecated. Please override
+ * @link #createTable(TableInfo) instead.
Review Comment:
I removed the deprecation, but the Javadoc was stale. fixed.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2001406072
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,15 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
I think we need to make schema a required column. `withXXX()` is for optional fields.
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978118012
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
Review Comment:
If this builder is only for creating tables, shall we rename it as `CreateTableBuilder` to improve clarity?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978917410
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
Maybe we should add extra method. e.g. `TableBuilder addProperty(String key, String val);`.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006635583
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -33,11 +33,11 @@
/**
* Catalog methods for working with Tables.
* <p>
- * TableCatalog implementations may be case sensitive or case insensitive. Spark will pass
+ * TableCatalog implementations may be case-sensitive or case-insensitive. Spark will pass
Review Comment:
It bothered me and I didn't want to raise a PR to fix this.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006503758
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
Review Comment:
Should this be null or empty by default? What do we pass now?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006506554
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateTableExec.scala:
##########
@@ -43,7 +43,12 @@ case class CreateTableExec(
override protected def run(): Seq[InternalRow] = {
if (!catalog.tableExists(identifier)) {
try {
- catalog.createTable(identifier, columns, partitioning.toArray, tableProperties.asJava)
+ val tableInfo = new TableInfo.Builder()
Review Comment:
Are there more places that should be migrated? What about CTAS?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009945046
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
+ return columns;
+ }
+
+ public Map<String, String> properties() {
+ return properties;
+ }
+
+ public Transform[] partitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
+ private Transform[] partitions;
+
+ public Builder withColumns(Column[] columns) {
Review Comment:
Let's also add `withSchema(StructType schema)` to be fully on par with the previous `createTable` overloads
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
+ return columns;
+ }
+
+ public Map<String, String> properties() {
+ return properties;
+ }
+
+ public Transform[] partitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
+ private Transform[] partitions;
+
+ public Builder withColumns(Column[] columns) {
Review Comment:
Let's also add `withSchema(StructType schema)` to be fully on par with the previous `createTable` overloads
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2011179150
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,90 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+import org.apache.spark.sql.types.StructType;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
+ return columns;
+ }
+
+ public StructType schema() {
+ return CatalogV2Util.v2ColumnsToStructType(columns);
+ }
+
+ public Map<String, String> properties() {
+ return properties;
+ }
+
+ public Transform[] partitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
+ private Transform[] partitions;
+
+ public Builder withColumns(Column[] columns) {
+ this.columns = columns;
+ return this;
+ }
+
+ public Builder withSchema(StructType schema) {
Review Comment:
no we don't need this. The builder is used by Spark and Spark should not use the deprecated StructType schema to create TableInfo.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2701218191
Can we put the new API spec in the PR description? and also mention how replace table will be supported. It's still not very clear to me what users need to implement.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980076364
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
Review Comment:
Done
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980125783
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
Review Comment:
Do we need to make this class public? I think we want external connectors to provide their own builders and rely on this one purely for backward compatibility.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
Review Comment:
Optional: I am personally prefer direct imports for nested classes/interfaces to shorten lines, if the context of the imported class is obvious.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
+1 to reusing it for replace.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -152,6 +152,66 @@ class CatalogSuite extends SparkFunSuite {
assert(catalog.tableExists(testIdent))
}
+ test("createTable: using builder non-partitioned table") {
+ val catalog = newCatalog()
+
+ assert(!catalog.tableExists(testIdent))
+
+ val table = catalog.buildTable(testIdent, columns)
+ .withPartitions(emptyTrans)
Review Comment:
Nit: Formatting is off.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
Review Comment:
Optional: I think the variables are well-named and descriptive enough, I am not sure there is much value in the extra comments.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
Review Comment:
I would say we have to keep the existing behavior, which I believe passes an empty array for unpartitioned tables.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
We can add new methods like `addProperty` once there is a use case for it. I do agree with Szehon it should replace the original map, like the PR currently does.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
Review Comment:
Question: What's the Java style we follow for formatting parameters in Java? I see it is not very consistent in the code but it mostly matches what we do for Scala where the first parameter is on a new line.
```
public TableBuilderImpl(
TableCatalog catalog,
Identifier identifier,
Column[] columns) {
this.catalog = catalog;
this.identifier = identifier;
this.columns = columns;
}
```
@cloud-fan @gengliangwang @szehon-ho, any input?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1998623775
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilder.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Builder for doing table creations. Spark gets the builder from the {@link TableCatalog},
+ * calls the `withXXX` methods to set up the fields and calls the `create` method.
+ */
+public class TableBuilder {
+ protected final TableCatalog catalog;
+
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties = Maps.newHashMap();
+
+ protected Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilder.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilder(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
Review Comment:
```suggestion
public TableBuilder(TableCatalog catalog, Identifier identifier, Column[] columns) {
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1998617041
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilder.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Builder for doing table creations. Spark gets the builder from the {@link TableCatalog},
Review Comment:
`Builder for creating tables.`
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006498909
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.identifier = builder.identifier;
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Identifier getIdentifier() {
+ return identifier;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public Builder toBuilder() {
+ return new Builder(identifier, columns)
+ .withProperties(properties)
+ .withPartitions(partitions);
+ }
+
+ public static class Builder {
Review Comment:
This makes sense to me.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006494771
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -230,6 +235,23 @@ default Table createTable(
return createTable(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
}
+ /**
+ * Create a table in the catalog.
+ *
+ * @param ident a table identifier
+ * @param tableInfo Information about the table.
Review Comment:
Minor: `Information` -> `information` to be consistent with the rest of the class?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005740423
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.identifier = builder.identifier;
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Identifier getIdentifier() {
+ return identifier;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public Builder toBuilder() {
Review Comment:
It is the standard builder pattern, where users can construct another POJO quickly with a different value for a field. I can remove it.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005729537
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.identifier = builder.identifier;
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Identifier getIdentifier() {
+ return identifier;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public Builder toBuilder() {
Review Comment:
why do we need this API?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006505090
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
Review Comment:
Optional: I am personally not a big fan of `getXXX` prefixes as they have no meaning. But that's usually something decided in each project. I wonder what @cloud-fan thinks here.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006502074
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
Review Comment:
Why make protected if we expose getters? Let's also skip an empty line between each field.
```
private final Column[] columns;
private final Map<String, String> properties;
private final Transform[] partitions;
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1999991291
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilder.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Builder for doing table creations. Spark gets the builder from the {@link TableCatalog},
+ * calls the `withXXX` methods to set up the fields and calls the `create` method.
+ */
+public class TableBuilder {
+ protected final TableCatalog catalog;
+
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties = Maps.newHashMap();
+
+ protected Transform[] partitions = new Transform[0];
Review Comment:
OK. I'm missing some comments.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2739209199
@cloud-fan @aokolnychyi New PR with the POJO. PTAL.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1981592239
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
Review Comment:
Yea this is the preferred java style AFAIK.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980124261
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
Review Comment:
Optional: I personally prefer direct imports for nested classes/interfaces to shorten lines, if the context of the imported class is obvious.
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978118459
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
How about `createTableBuilder`?
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978550542
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
I think we will reuse it for replace table
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978551802
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
Review Comment:
to make it clear that it build table metadata, not table data, shall we name it `TableInfoBuilder`?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1983880092
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
+1 with @szehon-ho @aokolnychyi @beliefer . We can explicitly mention that this API is for set/replace the whole properties in code comments.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1999126522
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilder.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Builder for doing table creations. Spark gets the builder from the {@link TableCatalog},
+ * calls the `withXXX` methods to set up the fields and calls the `create` method.
+ */
+public class TableBuilder {
+ protected final TableCatalog catalog;
+
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties = Maps.newHashMap();
+
+ protected Transform[] partitions = new Transform[0];
Review Comment:
We need to keep the existing behavior, which passes an empty array for unpartitioned tables. Please see the above review comment.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2726783327
@cloud-fan @aokolnychyi I have updated the PR based on Wenchen's suggestion. Please note that I made `TableBuilder` a concrete class so that we can have a default implementation for connectors who don't want to override anything. But that meant that the `TableCatalog` needs to be a constructor argument to the builder. The alternative would have been to make it an abstract class and having a concrete default implementation.
PTAL.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005732847
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -230,6 +235,23 @@ default Table createTable(
return createTable(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
}
+ /**
+ * Create a table in the catalog.
+ *
+ * @param tableInfo Information about the table.
+ * @return metadata for the new table. This can be null if getting the metadata for the new table
+ * is expensive. Spark will call {@link #loadTable(Identifier)} if needed (e.g. CTAS).
+ *
+ * @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 Table createTable(TableInfo tableInfo)
Review Comment:
I prefer to put `Identifier` as a parameter here instead of in `TableInfo`. This is to be consistent with most of the functions in this interface: `dropTable`, `alterTable`, `loadTable`, etc. which all have `Identifier` as its first parameter.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005742019
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/CatalogSuite.scala:
##########
@@ -75,7 +75,10 @@ class CatalogSuite extends SparkFunSuite {
intercept[NoSuchNamespaceException](catalog.listTables(Array("ns")))
- catalog.createTable(ident1, columns, emptyTrans, emptyProps)
+ val tableInfo = new TableInfo.Builder(ident1, columns)
+ .withPartitions(emptyTrans)
Review Comment:
That is correct, but I prefer to be explicit in the test.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006635112
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateTableExec.scala:
##########
@@ -43,7 +43,12 @@ case class CreateTableExec(
override protected def run(): Seq[InternalRow] = {
if (!catalog.tableExists(identifier)) {
try {
- catalog.createTable(identifier, columns, partitioning.toArray, tableProperties.asJava)
+ val tableInfo = new TableInfo.Builder()
Review Comment:
Migrated all v2 operators.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1981172203
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
Personally, I guess `TableBuilder addProperty(String key, String val);` will be used when updating table.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978906318
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
Review Comment:
Shall we add `Identifier ident();` into `TableBuilder` ?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2004834646
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.identifier = builder.identifier;
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Identifier getIdentifier() {
+ return identifier;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public Builder toBuilder() {
+ return new Builder(identifier, columns)
+ .withProperties(properties)
+ .withPartitions(partitions);
+ }
+
+ public static class Builder {
Review Comment:
Note that I added a builder for the POJO to avoid telescopic constructors.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005805967
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -230,6 +235,23 @@ default Table createTable(
return createTable(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
}
+ /**
+ * Create a table in the catalog.
+ *
+ * @param tableInfo Information about the table.
+ * @return metadata for the new table. This can be null if getting the metadata for the new table
+ * is expensive. Spark will call {@link #loadTable(Identifier)} if needed (e.g. CTAS).
+ *
+ * @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 Table createTable(TableInfo tableInfo)
Review Comment:
Sure.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980002481
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
Review Comment:
@cloud-fan What do you think?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1979992466
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
Wenchen is spot on. We are planning to reuse the interface for replace also (future PR).
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
Review Comment:
Addressed below.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980111812
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.columns = columns;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withProperties(Map<String, String> properties) {
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ @Override
+ public Table create() throws TableAlreadyExistsException, NoSuchNamespaceException {
+ return catalog.createTable(identifier, columns, partitions, properties);
Review Comment:
@anoopj and I replied at the same time :)
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1998626532
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilder.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Builder for doing table creations. Spark gets the builder from the {@link TableCatalog},
+ * calls the `withXXX` methods to set up the fields and calls the `create` method.
+ */
+public class TableBuilder {
+ protected final TableCatalog catalog;
+
+ protected final Identifier identifier;
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties = Maps.newHashMap();
+
+ protected Transform[] partitions = new Transform[0];
Review Comment:
I think we just need to use null here.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1999997357
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,15 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
Review Comment:
I guess we need a `buildTable` without parameters.
`default TableBuilder buildTable()`
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009950390
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/ReplaceTableExec.scala:
##########
@@ -18,12 +18,11 @@
package org.apache.spark.sql.execution.datasources.v2
import scala.jdk.CollectionConverters._
-
Review Comment:
unnecessary change
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009949026
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
+ return columns;
+ }
+
+ public Map<String, String> properties() {
+ return properties;
+ }
+
+ public Transform[] partitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
+ private Transform[] partitions;
+
+ public Builder withColumns(Column[] columns) {
+ this.columns = columns;
+ return this;
+ }
+
+ public Builder withProperties(Map<String, String> properties) {
+ this.properties = Maps.newHashMap();
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ public Builder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ public TableInfo build() {
+ return new TableInfo(this);
Review Comment:
shall we add non-null `assert` here as Spark should call all `withXXX` methods to set all table fields?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2009939605
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -230,6 +230,23 @@ default Table createTable(
return createTable(ident, CatalogV2Util.v2ColumnsToStructType(columns), partitions, properties);
}
+ /**
+ * Create a table in the catalog.
+ *
+ * @param ident a table identifier
+ * @param tableInfo information about the table.
+ * @return metadata for the new table. This can be null if getting the metadata for the new table
+ * is expensive. Spark will call {@link #loadTable(Identifier)} if needed (e.g. CTAS).
+ *
+ * @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 Table createTable(Identifier ident, TableInfo tableInfo)
Review Comment:
shall we deprecate the other overloads of `createTable`?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2704514930
> I think we just need to have an interface to hold all table information, and let createTable/replaceTable take it instead of many parameters.
Thanks for the feedback. Your suggestion seems simpler.
@aokolnychyi What do you think?
> ```
> interface TableInfo {
> Column[] columns;
> Transform[] partitions;
> ...
> }
This could just be an inheritable POJO instead?
>
> interface TableCatalog ... {
> Table createTable(Identifier ident, TableInfo t);
> }
> ```
Would we deprecate the existing `create/replace Table()` methods (In `TableCatalog` and `StagingTableCatalog`) for a new one that takes in `TableInfo` instead?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2705425290
> Would we deprecate the existing create/replace Table() methods (In TableCatalog and StagingTableCatalog) for a new one that takes in TableInfo instead?
Yea this is expected. In the future we don't need to update create/replaceTable() methods anymore, but `TableInfo`
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006488851
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -210,6 +210,10 @@ default Table createTable(
/**
* Create a table in the catalog.
+ * <p>
+ * @deprecated This is deprecated. Please override
+ * @link #createTable(TableInfo) instead.
Review Comment:
I believe the Javadoc is broken here and the reference is broken here. I also think the deprecate clause is usually is the last one at the bottom.
```
@throws ...
@throws ...
@deprecated Override {@link #createTable(Identifier, TableInfo)} instead.
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006488851
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -210,6 +210,10 @@ default Table createTable(
/**
* Create a table in the catalog.
+ * <p>
+ * @deprecated This is deprecated. Please override
+ * @link #createTable(TableInfo) instead.
Review Comment:
I believe the Javadoc formatting the reference are broken here. I also think the deprecate clause is usually is the last one at the bottom.
```
@throws ...
@throws ...
@deprecated Override {@link #createTable(Identifier, TableInfo)} instead.
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2010340747
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ private final Column[] columns;
+ private final Map<String, String> properties;
+ private final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] columns() {
+ return columns;
+ }
+
+ public Map<String, String> properties() {
+ return properties;
+ }
+
+ public Transform[] partitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
+ private Transform[] partitions;
+
+ public Builder withColumns(Column[] columns) {
+ this.columns = columns;
+ return this;
+ }
+
+ public Builder withProperties(Map<String, String> properties) {
+ this.properties = Maps.newHashMap();
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ public Builder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ public TableInfo build() {
+ return new TableInfo(this);
Review Comment:
I added a non-null precondition check for columns. The other fields could be `null` to be compatible with the current code.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2753324634
thanks, merging to master!
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005806582
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -222,6 +226,7 @@ default Table createTable(
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
+ @Deprecated(since = "4.0.0")
Review Comment:
SG
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50137: [SPARK-51372][SQL] Introduce TableInfo for table creations
URL: https://github.com/apache/spark/pull/50137
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006646605
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
+
+ protected final Map<String, String> properties;
+
+ protected final Transform[] partitions;
+
+ /**
+ * Constructor for TableInfo used by the builder.
+ * @param builder Builder.
+ */
+ private TableInfo(Builder builder) {
+ this.columns = builder.columns;
+ this.properties = Collections.unmodifiableMap(builder.properties);
+ this.partitions = builder.partitions;
+ }
+
+ public Column[] getColumns() {
+ return columns;
+ }
+
+ public Map<String, String> getProperties() {
+ return properties;
+ }
+
+ public Transform[] getPartitions() {
+ return partitions;
+ }
+
+ public static class Builder {
+ private Column[] columns;
+ private Map<String, String> properties;
Review Comment:
As per the current API contract, they could be `null`, but Spark never seems to send in nulls. I will make them `null` by default for consistency with existing code.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006508242
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -33,11 +33,11 @@
/**
* Catalog methods for working with Tables.
* <p>
- * TableCatalog implementations may be case sensitive or case insensitive. Spark will pass
+ * TableCatalog implementations may be case-sensitive or case-insensitive. Spark will pass
Review Comment:
These seem cosmetic and unrelated. I'd probably not mix them with adding `TableInfo`.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2005720937
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -222,6 +226,7 @@ default Table createTable(
* @throws UnsupportedOperationException If a requested partition transform is not supported
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
+ @Deprecated(since = "4.0.0")
Review Comment:
This is a backward compatible API refactor, shall we just put it in 4.1.0?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006502074
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
Review Comment:
Why make protected if we expose getters? Let's also skip an empty line between each field, it is not like we have a lot of fields and have to group them.
```
private final Column[] columns;
private final Map<String, String> properties;
private final Transform[] partitions;
```
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableInfo.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Collections;
+import java.util.Map;
+
+public class TableInfo {
+
+ protected final Column[] columns;
Review Comment:
Why make protected if we expose getters? Let's also skip an empty line between each field, it is not like we have a lot of fields and have to divide them into groups.
```
private final Column[] columns;
private final Map<String, String> properties;
private final Transform[] partitions;
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce TableInfo for table creations [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r2006488851
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -210,6 +210,10 @@ default Table createTable(
/**
* Create a table in the catalog.
+ * <p>
+ * @deprecated This is deprecated. Please override
+ * @link #createTable(TableInfo) instead.
Review Comment:
I believe the Javadoc formatting and the reference are broken here. I also think the deprecate clause is usually is the last one at the bottom.
```
@throws ...
@throws ...
@deprecated Override {@link #createTable(Identifier, TableInfo)} instead.
```
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2721388429
OK so the requirement is:
- adding a new item to table metadata should not require a new overload of `def createTable`
- the existing `def createTable` implementation should fail if a new table feature is added, but should still work if the new feature is not used
I think the builder should not be an interface, but a class that can have member variables and methods:
```
class CreateTableBuilder {
protected Column[] columns;
...
CreateTableBuilder withColumns ...
...
Table create...
}
```
Then we add a new method in `TableCatalog` to create a builder, with the default implementation to return the builtin builder.
The workflow is Spark getting the builder from `TableCatalog`, calling `withXXX` methods to set up fields, and calling `create` at the end.
When we add a new item (e.g. constraints), we add a new `withXXX` method and `supportsXXX` method in the builder. By default `supportsXXX` returns false, and `withXXX` throws an exception if `supportsXXX` is false. Users need to explicitly override `supportsXXX` to return true to adopt the new feature.
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on PR #50137:
URL: https://github.com/apache/spark/pull/50137#issuecomment-2701473872
> Can we put the new API spec in the PR description? and also mention how replace table will be supported. It's still not very clear to me what users need to implement.
Done. PTAL if that looks reasonable.
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978090554
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/CreateTableExec.scala:
##########
@@ -43,7 +43,10 @@ case class CreateTableExec(
override protected def run(): Seq[InternalRow] = {
if (!catalog.tableExists(identifier)) {
try {
- catalog.createTable(identifier, columns, partitioning.toArray, tableProperties.asJava)
+ catalog.buildTable(identifier, columns)
Review Comment:
I think Spark uses 2 space indents
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -294,6 +295,51 @@ class InMemoryTableCatalog extends BasicInMemoryTableCatalog with SupportsNamesp
}
case class Result(readSchema: StructType, rows: Array[InternalRow]) extends LocalScan
+
+ override def buildTable(ident: Identifier, columns: Array[Column]): TableBuilder = {
+ new BasicInMemoryTableBuilder(ident, columns, namespaces, tables)
+ }
+
+ private class BasicInMemoryTableBuilder(
+ val ident: Identifier,
Review Comment:
We can remove 'val' in these variables.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
It seems 'withProperties' sounds like it should set/replace the existing map, rather than increment the existing map, based on how the other method is defined.
In Spark the builder arguments for conf seem to take in single key, value. I wonder if we should follow this pattern if the goal is to have an additive API , else it seems wasteful (callers have to make a temporary java Map just to call this).
Maybe we can support both APIs?
Just my observation.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
Review Comment:
Nit: extra space
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -294,6 +295,51 @@ class InMemoryTableCatalog extends BasicInMemoryTableCatalog with SupportsNamesp
}
case class Result(readSchema: StructType, rows: Array[InternalRow]) extends LocalScan
+
+ override def buildTable(ident: Identifier, columns: Array[Column]): TableBuilder = {
+ new BasicInMemoryTableBuilder(ident, columns, namespaces, tables)
+ }
+
+ private class BasicInMemoryTableBuilder(
+ val ident: Identifier,
+ val columns: Array[Column],
+ val namespaces: util.Map[List[String], Map[String, String]],
+ val tables: util.Map[Identifier, Table])
Review Comment:
I think we dont need this?
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/connector/catalog/InMemoryTableCatalog.scala:
##########
@@ -294,6 +295,51 @@ class InMemoryTableCatalog extends BasicInMemoryTableCatalog with SupportsNamesp
}
case class Result(readSchema: StructType, rows: Array[InternalRow]) extends LocalScan
+
+ override def buildTable(ident: Identifier, columns: Array[Column]): TableBuilder = {
+ new BasicInMemoryTableBuilder(ident, columns, namespaces, tables)
+ }
+
+ private class BasicInMemoryTableBuilder(
+ val ident: Identifier,
+ val columns: Array[Column],
+ val namespaces: util.Map[List[String], Map[String, String]],
+ val tables: util.Map[Identifier, Table])
+ extends TableBuilder {
+ import org.apache.spark.sql.connector.catalog.CatalogV2Implicits._
+
+ private val properties: util.Map[String, String] = new util.HashMap();
Review Comment:
I think we can just define these defaults in the class definition.
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978292403
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
Review Comment:
Note: I guess you were trying to align the params? But I think the other method javadocs here dont align if they are on the same line.
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableCatalog.java:
##########
@@ -311,4 +311,49 @@ default boolean purgeTable(Identifier ident) throws UnsupportedOperationExceptio
*/
void renameTable(Identifier oldIdent, Identifier newIdent)
throws NoSuchTableException, TableAlreadyExistsException;
+
+ /**
+ * Instantiate a builder to create a table in the catalog.
+ *
+ * @param ident a table identifier.
+ * @param columns the columns of the new table.
+ * @return the TableBuilder to create a table.
+ */
+ default TableBuilder buildTable(Identifier ident, Column[] columns) {
+ return new TableBuilderImpl(this, ident, columns);
+ }
+
+ /**
+ * Builder used to create tables.
+ *
+ * <p>Call {@link #buildTable(Identifier, Column[])} to create a new builder.
+ */
+ interface TableBuilder {
+ /**
+ * Sets the partitions for the table.
+ *
+ * @param partitions Partitions for the table.
+ * @return this for method chaining
+ */
+ TableBuilder withPartitions(Transform[] partitions);
+
+ /**
+ * Adds key/value properties to the table.
+ *
+ * @param properties key/value properties
+ * @return this for method chaining
+ */
+ TableBuilder withProperties(Map<String, String> properties);
Review Comment:
It seems 'withProperties' sounds like it should set/replace the existing map, rather than increment the existing map, based on how the other method is defined.
In Spark the builder arguments for conf seem to take in single key, value. I wonder if we should follow this pattern if the goal is to have an additive API , else it seems wasteful (callers have to make a temporary java Map just to call this).
Maybe we can support both APIs?
Just my thought.
--
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
Re: [PR] [SPARK-51372] Introduce a builder pattern in TableCatalog [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1978552750
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.columns = columns;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withProperties(Map<String, String> properties) {
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ @Override
+ public Table create() throws TableAlreadyExistsException, NoSuchNamespaceException {
+ return catalog.createTable(identifier, columns, partitions, properties);
Review Comment:
I'm confused, how does it help if we do want to add a new parameter? Do we still need to add a new overload of `createTable`?
--
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
Re: [PR] [SPARK-51372][SQL] Introduce a builder pattern in TableCatalog [spark]
Posted by "anoopj (via GitHub)" <gi...@apache.org>.
anoopj commented on code in PR #50137:
URL: https://github.com/apache/spark/pull/50137#discussion_r1980110293
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/TableBuilderImpl.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.spark.sql.connector.catalog;
+
+import com.google.common.collect.Maps;
+import org.apache.spark.sql.catalyst.analysis.NoSuchNamespaceException;
+import org.apache.spark.sql.catalyst.analysis.TableAlreadyExistsException;
+import org.apache.spark.sql.connector.expressions.Transform;
+
+import java.util.Map;
+
+/**
+ * Default implementation of {@link TableCatalog.TableBuilder}.
+ */
+public class TableBuilderImpl implements TableCatalog.TableBuilder {
+ /** Catalog where table needs to be created. */
+ private final TableCatalog catalog;
+ /** Table identifier. */
+ private final Identifier identifier;
+ /** Columns of the new table. */
+ private final Column[] columns;
+ /** Table properties. */
+ private final Map<String, String> properties = Maps.newHashMap();
+ /** Transforms to use for partitioning data in the table. */
+ private Transform[] partitions = new Transform[0];
+
+ /**
+ * Constructor for TableBuilderImpl.
+ *
+ * @param catalog catalog where table needs to be created.
+ * @param identifier identifier for the table.
+ * @param columns the columns of the new table.
+ */
+ public TableBuilderImpl(TableCatalog catalog,
+ Identifier identifier,
+ Column[] columns) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.columns = columns;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withPartitions(Transform[] partitions) {
+ this.partitions = partitions;
+ return this;
+ }
+
+ @Override
+ public TableCatalog.TableBuilder withProperties(Map<String, String> properties) {
+ this.properties.putAll(properties);
+ return this;
+ }
+
+ @Override
+ public Table create() throws TableAlreadyExistsException, NoSuchNamespaceException {
+ return catalog.createTable(identifier, columns, partitions, properties);
Review Comment:
This default implementation is to offer backward compatibility (so we can migrate existing code incrementally). Please let me know if you have suggestions on how to do this better.
--
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