[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317420150
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,238 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest extends HiveMetastoreTest {
+
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+exceptionRule.expectMessage("Metastore operation failed");
+
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+
+// create the table concurrently
+catalog.createTable(TABLE_IDENTIFIER, SCHEMA, SPEC);
+Assert.assertTrue("Table should be created",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// expect the transaction to fail
+txn.commitTransaction();
+ }
+
+ @Test
+ public void testCreateTableTxnTableAlreadyExists() {
+exceptionRule.expect(AlreadyExistsException.class);
+exceptionRule.expectMessage("Table already exists: hivedb.tbl");
+
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// create a table before starting a transaction
+catalog.createTable(TABLE_IDENTIFIER, SCHEMA, SPEC);
+Assert.assertTrue("Table should be created",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// should not be possible to start a new create table transaction
+catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+ }
+
+ @Test
+ public void testReplaceTableTxn() {
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317420104
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,238 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest extends HiveMetastoreTest {
+
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+exceptionRule.expectMessage("Metastore operation failed");
+
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+
+// create the table concurrently
+catalog.createTable(TABLE_IDENTIFIER, SCHEMA, SPEC);
+Assert.assertTrue("Table should be created",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// expect the transaction to fail
+txn.commitTransaction();
+ }
+
+ @Test
+ public void testCreateTableTxnTableAlreadyExists() {
+exceptionRule.expect(AlreadyExistsException.class);
+exceptionRule.expectMessage("Table already exists: hivedb.tbl");
+
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// create a table before starting a transaction
+catalog.createTable(TABLE_IDENTIFIER, SCHEMA, SPEC);
+Assert.assertTrue("Table should be created",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// should not be possible to start a new create table transaction
+catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+ }
+
+ @Test
+ public void testReplaceTableTxn() {
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317420055
##
File path: hive/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java
##
@@ -184,6 +183,9 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
});
}
threw = false;
+} catch (org.apache.hadoop.hive.metastore.api.AlreadyExistsException e) {
Review comment:
This part requires attention. This change was needed to always throw
`AlreadyExistsException` instead of `RuntimeException` when tables are created
concurrently.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317419683
##
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##
@@ -241,7 +234,14 @@ private void commitCreateTransaction() {
}
}
- private void commitReplaceTransaction() {
+ private void commitReplaceTransaction(boolean orCreate) {
+if (base == null && !orCreate) {
+ throw new NoSuchTableException("Table doesn't exist");
+} else if (base == null) {
+ commitCreateTransaction();
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317387008
##
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##
@@ -241,7 +234,14 @@ private void commitCreateTransaction() {
}
}
- private void commitReplaceTransaction() {
+ private void commitReplaceTransaction(boolean orCreate) {
+if (base == null && !orCreate) {
+ throw new NoSuchTableException("Table doesn't exist");
+} else if (base == null) {
+ commitCreateTransaction();
Review comment:
We need to know how many retries to use, etc. We cannot configure `Tasks` as
`base` is null.
```
Tasks.foreach(ops)
.retry(base.propertyAsInt(COMMIT_NUM_RETRIES,
COMMIT_NUM_RETRIES_DEFAULT))
.exponentialBackoff(
base.propertyAsInt(COMMIT_MIN_RETRY_WAIT_MS,
COMMIT_MIN_RETRY_WAIT_MS_DEFAULT),
base.propertyAsInt(COMMIT_MAX_RETRY_WAIT_MS,
COMMIT_MAX_RETRY_WAIT_MS_DEFAULT),
base.propertyAsInt(COMMIT_TOTAL_RETRY_TIME_MS,
COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT),
2.0 /* exponential */)
.onlyRetryOn(CommitFailedException.class)
.run(underlyingOps -> {
...
}
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317377271
##
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##
@@ -241,7 +234,14 @@ private void commitCreateTransaction() {
}
}
- private void commitReplaceTransaction() {
+ private void commitReplaceTransaction(boolean orCreate) {
+if (base == null && !orCreate) {
+ throw new NoSuchTableException("Table doesn't exist");
+} else if (base == null) {
+ commitCreateTransaction();
Review comment:
If `base` is null, I think we can either use `replaceMetadata` instead of
`base` and keep the existing try-catch or we can actually call
`commitCreateTransaction` wrapped in another try first. If we get
`AlreadyExistsException`, we can call the second one, which will replace the
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317373320
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,238 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest extends HiveMetastoreTest {
+
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+exceptionRule.expectMessage("Metastore operation failed");
+
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+
+// create the table concurrently
+catalog.createTable(TABLE_IDENTIFIER, SCHEMA, SPEC);
+Assert.assertTrue("Table should be created",
catalog.tableExists(TABLE_IDENTIFIER));
+
+// expect the transaction to fail
+txn.commitTransaction();
Review comment:
Wait, what is the difference between `AssertHelpers` and `TestHelpers`?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317372959
##
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##
@@ -241,7 +234,14 @@ private void commitCreateTransaction() {
}
}
- private void commitReplaceTransaction() {
+ private void commitReplaceTransaction(boolean orCreate) {
+if (base == null && !orCreate) {
+ throw new NoSuchTableException("Table doesn't exist");
+} else if (base == null) {
+ commitCreateTransaction();
Review comment:
Should we use `replaceMetadata` to get the number of retries if `base` is
null?
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r317372229
##
File path: core/src/main/java/org/apache/iceberg/BaseTransaction.java
##
@@ -241,7 +234,14 @@ private void commitCreateTransaction() {
}
}
- private void commitReplaceTransaction() {
+ private void commitReplaceTransaction(boolean orCreate) {
+if (base == null && !orCreate) {
+ throw new NoSuchTableException("Table doesn't exist");
Review comment:
Yeah, it is actually handled there as well. This should never happen.
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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r316228984
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,211 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest {
+
+ private static final String DB_NAME = "hivedb";
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ private static TestHiveMetastore metastore;
+ private static HiveMetaStoreClient metastoreClient;
+ private static HiveConf hiveConf;
+ private static HiveCatalog catalog;
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @BeforeClass
+ public static void startMetastore() throws Exception {
+HiveCreateReplaceTableTest.metastore = new TestHiveMetastore();
+metastore.start();
+HiveCreateReplaceTableTest.hiveConf = metastore.hiveConf();
+HiveCreateReplaceTableTest.metastoreClient = new
HiveMetaStoreClient(hiveConf);
+String dbPath = metastore.getDatabasePath(DB_NAME);
+Database db = new Database(DB_NAME, "description", dbPath, new
HashMap<>());
+metastoreClient.createDatabase(db);
+HiveCreateReplaceTableTest.catalog = new HiveCatalog(hiveConf);
+ }
+
+ @AfterClass
+ public static void stopMetastore() {
+catalog.close();
+HiveCreateReplaceTableTest.catalog = null;
+
+metastoreClient.close();
+HiveCreateReplaceTableTest.metastoreClient = null;
+
+metastore.stop();
+HiveCreateReplaceTableTest.metastore = null;
+ }
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r316210566
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,211 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest {
+
+ private static final String DB_NAME = "hivedb";
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ private static TestHiveMetastore metastore;
+ private static HiveMetaStoreClient metastoreClient;
+ private static HiveConf hiveConf;
+ private static HiveCatalog catalog;
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @BeforeClass
+ public static void startMetastore() throws Exception {
+HiveCreateReplaceTableTest.metastore = new TestHiveMetastore();
+metastore.start();
+HiveCreateReplaceTableTest.hiveConf = metastore.hiveConf();
+HiveCreateReplaceTableTest.metastoreClient = new
HiveMetaStoreClient(hiveConf);
+String dbPath = metastore.getDatabasePath(DB_NAME);
+Database db = new Database(DB_NAME, "description", dbPath, new
HashMap<>());
+metastoreClient.createDatabase(db);
+HiveCreateReplaceTableTest.catalog = new HiveCatalog(hiveConf);
+ }
+
+ @AfterClass
+ public static void stopMetastore() {
+catalog.close();
+HiveCreateReplaceTableTest.catalog = null;
+
+metastoreClient.close();
+HiveCreateReplaceTableTest.metastoreClient = null;
+
+metastore.stop();
+HiveCreateReplaceTableTest.metastore = null;
+ }
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r314714596
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,211 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest {
+
+ private static final String DB_NAME = "hivedb";
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ private static TestHiveMetastore metastore;
+ private static HiveMetaStoreClient metastoreClient;
+ private static HiveConf hiveConf;
+ private static HiveCatalog catalog;
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @BeforeClass
+ public static void startMetastore() throws Exception {
+HiveCreateReplaceTableTest.metastore = new TestHiveMetastore();
+metastore.start();
+HiveCreateReplaceTableTest.hiveConf = metastore.hiveConf();
+HiveCreateReplaceTableTest.metastoreClient = new
HiveMetaStoreClient(hiveConf);
+String dbPath = metastore.getDatabasePath(DB_NAME);
+Database db = new Database(DB_NAME, "description", dbPath, new
HashMap<>());
+metastoreClient.createDatabase(db);
+HiveCreateReplaceTableTest.catalog = new HiveCatalog(hiveConf);
+ }
+
+ @AfterClass
+ public static void stopMetastore() {
+catalog.close();
+HiveCreateReplaceTableTest.catalog = null;
+
+metastoreClient.close();
+HiveCreateReplaceTableTest.metastoreClient = null;
+
+metastore.stop();
+HiveCreateReplaceTableTest.metastore = null;
+ }
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r314315329
##
File path:
hive/src/test/java/org/apache/iceberg/hive/HiveCreateReplaceTableTest.java
##
@@ -0,0 +1,211 @@
+/*
+ * 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.iceberg.hive;
+
+import com.google.common.collect.Maps;
+import java.io.IOException;
+import java.util.HashMap;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.metastore.HiveMetaStoreClient;
+import org.apache.hadoop.hive.metastore.api.Database;
+import org.apache.iceberg.PartitionSpec;
+import org.apache.iceberg.Schema;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.Transaction;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.exceptions.AlreadyExistsException;
+import org.apache.iceberg.exceptions.NoSuchTableException;
+import org.apache.iceberg.types.Types;
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
+import org.junit.rules.TemporaryFolder;
+
+import static org.apache.iceberg.PartitionSpec.builderFor;
+import static org.apache.iceberg.types.Types.NestedField.required;
+
+public class HiveCreateReplaceTableTest {
+
+ private static final String DB_NAME = "hivedb";
+ private static final String TABLE_NAME = "tbl";
+ private static final TableIdentifier TABLE_IDENTIFIER =
TableIdentifier.of(DB_NAME, TABLE_NAME);
+ private static final Schema SCHEMA = new Schema(
+ required(3, "id", Types.IntegerType.get()),
+ required(4, "data", Types.StringType.get())
+ );
+ private static final PartitionSpec SPEC = builderFor(SCHEMA)
+ .identity("id")
+ .build();
+
+ private static TestHiveMetastore metastore;
+ private static HiveMetaStoreClient metastoreClient;
+ private static HiveConf hiveConf;
+ private static HiveCatalog catalog;
+
+ @Rule
+ public ExpectedException exceptionRule = ExpectedException.none();
+ @Rule
+ public TemporaryFolder temp = new TemporaryFolder();
+
+ private String tableLocation;
+
+ @BeforeClass
+ public static void startMetastore() throws Exception {
+HiveCreateReplaceTableTest.metastore = new TestHiveMetastore();
+metastore.start();
+HiveCreateReplaceTableTest.hiveConf = metastore.hiveConf();
+HiveCreateReplaceTableTest.metastoreClient = new
HiveMetaStoreClient(hiveConf);
+String dbPath = metastore.getDatabasePath(DB_NAME);
+Database db = new Database(DB_NAME, "description", dbPath, new
HashMap<>());
+metastoreClient.createDatabase(db);
+HiveCreateReplaceTableTest.catalog = new HiveCatalog(hiveConf);
+ }
+
+ @AfterClass
+ public static void stopMetastore() {
+catalog.close();
+HiveCreateReplaceTableTest.catalog = null;
+
+metastoreClient.close();
+HiveCreateReplaceTableTest.metastoreClient = null;
+
+metastore.stop();
+HiveCreateReplaceTableTest.metastore = null;
+ }
+
+ @Before
+ public void createTableLocation() throws IOException {
+tableLocation = temp.newFolder("hive-").getPath();
+ }
+
+ @After
+ public void cleanup() {
+catalog.dropTable(TABLE_IDENTIFIER);
+ }
+
+ @Test
+ public void testCreateTableTxn() {
+Assert.assertFalse("Table should not exist",
catalog.tableExists(TABLE_IDENTIFIER));
+
+Transaction txn = catalog.newCreateTableTransaction(
+TABLE_IDENTIFIER, SCHEMA, SPEC, tableLocation, Maps.newHashMap());
+txn.updateProperties()
+.set("prop", "value")
+.commit();
+
+// verify the table is still not visible before the transaction is
committed
+Assert.assertFalse(catalog.tableExists(TABLE_IDENTIFIER));
+
+txn.commitTransaction();
+
+Table table = catalog.loadTable(TABLE_IDENTIFIER);
+Assert.assertEquals("Table props should match", "value",
table.properties().get("prop"));
+ }
+
+ @Test
+ public void testCreateTableTxnTableCreatedConcurrently() {
+exceptionRule.expect(RuntimeException.class);
+
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r314314329
##
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##
@@ -97,6 +98,42 @@ default Table createTable(
return createTable(identifier, schema, PartitionSpec.unpartitioned(),
null, null);
}
+ /**
+ * Start a transaction to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @param spec a partition spec
+ * @param location a location for the table; leave null if unspecified
+ * @param properties a string map of table properties
+ * @return a {@link Transaction} to create the table
+ * @throws AlreadyExistsException if the table already exists
+ */
+ Transaction newCreateTableTransaction(
Review comment:
I'll add 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:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
[GitHub] [incubator-iceberg] aokolnychyi commented on a change in pull request #362: Support create and replace transactions in Catalog
aokolnychyi commented on a change in pull request #362: Support create and
replace transactions in Catalog
URL: https://github.com/apache/incubator-iceberg/pull/362#discussion_r311696942
##
File path: api/src/main/java/org/apache/iceberg/catalog/Catalog.java
##
@@ -97,6 +98,42 @@ default Table createTable(
return createTable(identifier, schema, PartitionSpec.unpartitioned(),
null, null);
}
+ /**
+ * Start a transaction to create a table.
+ *
+ * @param identifier a table identifier
+ * @param schema a schema
+ * @param spec a partition spec
+ * @param location a location for the table; leave null if unspecified
+ * @param properties a string map of table properties
+ * @return a {@link Transaction} to create the table
+ * @throws AlreadyExistsException if the table already exists
+ */
+ Transaction newCreateTableTransaction(
Review comment:
I am a bit concerned about the number of methods we will have in `Catalog`.
So, I did not include overloaded helper methods as we have for create table
statements. Let me know what other people 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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
