You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/09/01 16:04:37 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request #1409: API: Introduce a builder in Catalog
aokolnychyi opened a new pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409
This PR introduces a builder in `Catalog` to avoid duplicating the number of methods each time we introduce a new param.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jacques-n commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-686616025
> @jacques-n, I haven't seen immutables before. How is your impression? Is the naming of generated methods fixed?
I'm not sure whether you mean "fixed" as in broken or "fixed" as in not-customizable. If the former, I'm not sure what you're referring to. If the latter, yes there is some control to customize the method names:
https://immutables.github.io/style.html
We find that it is relatively painless to use, has good IDE integration and removes a lot of boilerplate code. Allows us to do a better job of creating domain specific model classes in different layers rather than over-sharing them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483041932
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +303,83 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to either create a table or start a create/replace transaction.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table or start a create/replace transaction
+ */
+ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+ throw new UnsupportedOperationException("Not implemented: buildTable");
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #buildTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ interface TableBuilder {
Review comment:
Okay, I added validation for `HadoopCatalog` but we can add support for `buildTable` in `CachingCatalog` later. Looks like we will need a separate builder implementation that would delegate to another builder but add caching callbacks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r481407456
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
+ return new Builder(this, identifier, schema);
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #newTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ class Builder {
+ private final Catalog catalog;
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final Map<String, String> properties = new HashMap<>();
Review comment:
What about `ImmutableMap.Builder`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483042362
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -50,30 +50,12 @@ public Table createTable(
PartitionSpec spec,
String location,
Map<String, String> properties) {
- Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
Review comment:
I moved this logic to the builder and use it 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483042851
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -212,6 +183,114 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier tableIdentifier);
+ protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ protected BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
Review comment:
We did not validate this for replace transactions before but there should be no harm.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483049320
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +303,83 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to either create a table or start a create/replace transaction.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table or start a create/replace transaction
+ */
+ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+ throw new UnsupportedOperationException(this.getClass().getName() + " does not implement buildTable");
Review comment:
Sounds good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r482715860
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +303,83 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to either create a table or start a create/replace transaction.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table or start a create/replace transaction
+ */
+ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+ throw new UnsupportedOperationException("Not implemented: buildTable");
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #buildTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ interface TableBuilder {
Review comment:
I'll need to fix CachingCatalog.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r481397074
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
+ return new Builder(this, identifier, schema);
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #newTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ class Builder {
+ private final Catalog catalog;
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final Map<String, String> properties = new HashMap<>();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ private Builder(Catalog catalog, TableIdentifier identifier, Schema schema) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ /**
+ * Sets a partition spec for the table.
+ *
+ * @param newSpec a partition spec
+ * @return this for method chaining
+ */
+ public Builder withPartitionSpec(PartitionSpec newSpec) {
+ this.spec = newSpec;
+ return this;
+ }
+
+ /**
+ * Sets a location for the table.
+ *
+ * @param newLocation a location
+ * @return this for method chaining
+ */
+ public Builder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ /**
+ * Adds a key/value property to the table.
+ *
+ * @param key a key
+ * @param value a value
+ * @return this for method chaining
+ */
+ public Builder withProperty(String key, String value) {
Review comment:
We probably need to expose `withProperties` that accepts a map.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r481397074
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
+ return new Builder(this, identifier, schema);
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #newTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ class Builder {
+ private final Catalog catalog;
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final Map<String, String> properties = new HashMap<>();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ private Builder(Catalog catalog, TableIdentifier identifier, Schema schema) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ /**
+ * Sets a partition spec for the table.
+ *
+ * @param newSpec a partition spec
+ * @return this for method chaining
+ */
+ public Builder withPartitionSpec(PartitionSpec newSpec) {
+ this.spec = newSpec;
+ return this;
+ }
+
+ /**
+ * Sets a location for the table.
+ *
+ * @param newLocation a location
+ * @return this for method chaining
+ */
+ public Builder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ /**
+ * Adds a key/value property to the table.
+ *
+ * @param key a key
+ * @param value a value
+ * @return this for method chaining
+ */
+ public Builder withProperty(String key, String value) {
Review comment:
In addition, we probably need to expose `withProperties` that accepts a map.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483045426
##########
File path: core/src/main/java/org/apache/iceberg/hadoop/HadoopCatalog.java
##########
@@ -156,13 +154,6 @@ protected String name() {
return Lists.newArrayList(tblIdents);
}
- @Override
- public Table createTable(
Review comment:
This is now validated in the builder.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483058561
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -218,6 +189,114 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier tableIdentifier);
+ protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
+
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ @Override
+ public TableBuilder withPartitionSpec(PartitionSpec newSpec) {
+ if (spec != null) {
+ this.spec = newSpec;
+ }
+ return this;
+ }
+
+ @Override
+ public TableBuilder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ @Override
+ public TableBuilder withProperties(Map<String, String> properties) {
+ if (properties != null) {
Review comment:
We used to have this because overloaded createTable methods pass null for props:
```
Map<String, String> tableProperties = properties != null ? properties : Maps.newHashMap();
```
Since we are using the builder for the root `createTable` now, I added handling here too. For partition spec, it is optional but probably makes sense for consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483045822
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
Review comment:
Done.
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
+ return new Builder(this, identifier, schema);
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #newTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ class Builder {
+ private final Catalog catalog;
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final Map<String, String> properties = new HashMap<>();
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-685115239
I think there's a slight problem with the approach that this uses. This creates a builder that interacts with the public `Catalog` API. That's great for users of the API because it is easier to create tables using the builder. But it doesn't help implementers of the API or handle the problem that we keep adding new variations of `createTable` when we add new table metadata.
Since the builder ends up calling `createTable` with all of the metadata options it has accumulated, when we need to add a new one, like `distribute by` for example, we will still need to create another variant of `createTable` and update the builder to call it. So we will still accumulate more methods in the public API.
Ideally, the builder would create `TableMetadata` and pass it into the implementation. The problem is that `TableMetadata` is not an API class. So if we wanted to do that, we wouldn't be able to provide the builder implementation in the API. We would have to add an interface for the builder that is implemented in some `BaseCatalog` in `iceberg-core`. That's a bit more work, but I think it would help in the long term. What do you think @aokolnychyi?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483058561
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -218,6 +189,114 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier tableIdentifier);
+ protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
+
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ @Override
+ public TableBuilder withPartitionSpec(PartitionSpec newSpec) {
+ if (spec != null) {
+ this.spec = newSpec;
+ }
+ return this;
+ }
+
+ @Override
+ public TableBuilder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ @Override
+ public TableBuilder withProperties(Map<String, String> properties) {
+ if (properties != null) {
Review comment:
We used to have this because overloaded `createTable` methods pass null for props:
```
Map<String, String> tableProperties = properties != null ? properties : Maps.newHashMap();
```
Since we are using the builder for the root `createTable` now, I added handling here too. For partition spec, it is optional but probably makes sense for consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jacques-n commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
jacques-n commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-685116089
Random aside, have you guys considered using [Immutables?](https://immutables.github.io/)
We've started to use it extensively other places as it does a nice job of several things around pojo and builders. It's also nice as it is an annotation processor and creates no runtime dependency (but still produces inspectable code, as opposed to something like lombok, which is pretty much black box)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483050254
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -218,6 +189,114 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier tableIdentifier);
+ protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
+
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ @Override
+ public TableBuilder withPartitionSpec(PartitionSpec newSpec) {
+ if (spec != null) {
Review comment:
Why allow null here? We used to allow null for the parameter because you had to pass it, but now you don't have to pass it so maybe we should reject null?
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -218,6 +189,114 @@ public String toString() {
protected abstract String defaultWarehouseLocation(TableIdentifier tableIdentifier);
+ protected class BaseMetastoreCatalogTableBuilder implements TableBuilder {
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final ImmutableMap.Builder<String, String> propertiesBuilder = ImmutableMap.builder();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ public BaseMetastoreCatalogTableBuilder(TableIdentifier identifier, Schema schema) {
+ Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
+
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ @Override
+ public TableBuilder withPartitionSpec(PartitionSpec newSpec) {
+ if (spec != null) {
+ this.spec = newSpec;
+ }
+ return this;
+ }
+
+ @Override
+ public TableBuilder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ @Override
+ public TableBuilder withProperties(Map<String, String> properties) {
+ if (properties != null) {
Review comment:
Same null question 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-686557764
+1 when conflicts are addressed and tests are passing.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-686564541
@jacques-n, I haven't seen immutables before. How is your impression? Is the naming of generated methods 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483040698
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +303,83 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to either create a table or start a create/replace transaction.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table or start a create/replace transaction
+ */
+ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+ throw new UnsupportedOperationException(this.getClass().getName() + " does not implement buildTable");
Review comment:
I am throwing `UnsupportedOperationException` to avoid breaking custom catalogs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#issuecomment-684964432
@rdblue @RussellSpitzer @rdsr this is the builder we discussed [here](https://github.com/apache/iceberg/pull/1373/files#r475944439).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r481408453
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
Review comment:
What about `buildTable` instead? This doesn't really return a table, it returns a builder that will create either a `Table` or a `Transaction`.
We may also want to mention in Javadoc that this builder is used to either create a table or start a create or replace transaction.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483046021
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +304,105 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table
+ */
+ default Builder newTable(TableIdentifier identifier, Schema schema) {
+ return new Builder(this, identifier, schema);
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #newTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ class Builder {
+ private final Catalog catalog;
+ private final TableIdentifier identifier;
+ private final Schema schema;
+ private final Map<String, String> properties = new HashMap<>();
+ private PartitionSpec spec = PartitionSpec.unpartitioned();
+ private String location = null;
+
+ private Builder(Catalog catalog, TableIdentifier identifier, Schema schema) {
+ this.catalog = catalog;
+ this.identifier = identifier;
+ this.schema = schema;
+ }
+
+ /**
+ * Sets a partition spec for the table.
+ *
+ * @param newSpec a partition spec
+ * @return this for method chaining
+ */
+ public Builder withPartitionSpec(PartitionSpec newSpec) {
+ this.spec = newSpec;
+ return this;
+ }
+
+ /**
+ * Sets a location for the table.
+ *
+ * @param newLocation a location
+ * @return this for method chaining
+ */
+ public Builder withLocation(String newLocation) {
+ this.location = newLocation;
+ return this;
+ }
+
+ /**
+ * Adds a key/value property to the table.
+ *
+ * @param key a key
+ * @param value a value
+ * @return this for method chaining
+ */
+ public Builder withProperty(String key, String value) {
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi merged pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r483042362
##########
File path: core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java
##########
@@ -50,30 +50,12 @@ public Table createTable(
PartitionSpec spec,
String location,
Map<String, String> properties) {
- Preconditions.checkArgument(isValidIdentifier(identifier), "Invalid table identifier: %s", identifier);
Review comment:
I moved this logic to the builder and use the builder 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #1409: API: Introduce a builder in Catalog
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #1409:
URL: https://github.com/apache/iceberg/pull/1409#discussion_r482735007
##########
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##########
@@ -303,4 +303,83 @@ default boolean dropTable(TableIdentifier identifier) {
* @throws NoSuchTableException if the table does not exist
*/
Table loadTable(TableIdentifier identifier);
+
+ /**
+ * Instantiate a builder to either create a table or start a create/replace transaction.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @return the builder to create a table or start a create/replace transaction
+ */
+ default TableBuilder buildTable(TableIdentifier identifier, Schema schema) {
+ throw new UnsupportedOperationException("Not implemented: buildTable");
+ }
+
+ /**
+ * A builder used to create valid {@link Table tables}.
+ * <p>
+ * Call {@link #buildTable(TableIdentifier, Schema)} to create a new builder.
+ */
+ interface TableBuilder {
Review comment:
Also, we will need to add extra validation to the builder in `HadoopCatalog`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org