kbendick commented on a change in pull request #3275:
URL: https://github.com/apache/iceberg/pull/3275#discussion_r779861519
##########
File path: core/src/test/java/org/apache/iceberg/jdbc/TestJdbcCatalog.java
##########
@@ -555,6 +556,66 @@ public void testDropNamespace() {
"is not empty. 1 tables exist.", () ->
catalog.dropNamespace(tbl4.namespace()));
}
+ @Test
+ public void testCreateNamespace() {
+ Namespace testNamespace = Namespace.of("testDb", "ns1", "ns2");
+ // Test with null metadata
+ AssertHelpers.assertThrows("Cannot create a namespace with null or empty
metadata", IllegalArgumentException.class,
+ () -> catalog.createNamespace(testNamespace, null));
+ Assert.assertFalse(catalog.namespaceExists(testNamespace));
+
+ // Test with metadata
+ Map<String, String> testMetadata = ImmutableMap.of("key_1", "value_1",
"key_2", "value_2", "key_3", "value_3");
+ catalog.createNamespace(testNamespace, testMetadata);
+ Assert.assertTrue(catalog.namespaceExists(testNamespace));
+ }
+
+ @Test
+ public void testSetProperties() {
+ Namespace testNamespace = Namespace.of("testDb", "ns1", "ns2");
+ Map<String, String> testMetadata = ImmutableMap.of("key_1", "value_1",
"key_2", "value_2",
+ "key_3", "value_3");
+ catalog.createNamespace(testNamespace, testMetadata);
+
+ // Add more properties to set to test insert and update
+ Map<String, String> propertiesToSet = ImmutableMap.of("key_1",
"new_value_1", "key_2", "new_value_2", "key_3",
+ "new_value_3", "key_4", "value_4", "key_5", "value_5");
Review comment:
Can we add the properties to set in an order where the keys don't
already align with the ordering of the existing metadata?
I'm wondering if we need to add a check if we're at the right row number
when calling the `updatePrperties` function. I think even just removing `key_2`
from here would trigger this case.
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -250,10 +265,95 @@ public void setConf(Configuration conf) {
this.conf = conf;
}
+ private boolean insertProperties(Namespace namespace, Map<String, String>
properties) {
+ String namespaceName = JdbcUtil.namespaceToString(namespace);
+
+ try {
+ int insertedRecords = connections.run(conn -> {
+ String sqlStatement =
JdbcUtil.insertPropertiesStatement(properties.size());
+
+ try (PreparedStatement sql = conn.prepareStatement(sqlStatement)) {
+ int rowIndex = 0;
+ for (Map.Entry<String, String> keyValue : properties.entrySet()) {
+ sql.setString(rowIndex + 1, catalogName);
+ sql.setString(rowIndex + 2, namespaceName);
+ sql.setString(rowIndex + 3, keyValue.getKey());
+ sql.setString(rowIndex + 4, keyValue.getValue());
+ rowIndex += 4;
+ }
+ return sql.executeUpdate();
+ }
+ });
+
+ if (insertedRecords == properties.size()) {
+ LOG.debug("Successfully inserted {} properties for namespace {}",
properties, namespaceName);
+ return true;
+ } else {
+ throw new IllegalStateException();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedInterruptedException(e, "Interrupted in call to
insertProperties(namespace, properties) " +
+ "Namespace: %s", namespace);
+ } catch (SQLException e) {
+ throw new UncheckedSQLException(e, "Failed to insertProperties to
namespace: %s in catalog: %s", namespace,
Review comment:
Nit: We try to keep error messages as human-readable strings as opposed
to using function names as users likely won't know the names of functions
anyways (and they can be logged in stack traces).
So possibly this should be `Failed to insert properties to namespace ...`
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -250,10 +265,95 @@ public void setConf(Configuration conf) {
this.conf = conf;
}
+ private boolean insertProperties(Namespace namespace, Map<String, String>
properties) {
+ String namespaceName = JdbcUtil.namespaceToString(namespace);
+
+ try {
+ int insertedRecords = connections.run(conn -> {
+ String sqlStatement =
JdbcUtil.insertPropertiesStatement(properties.size());
+
+ try (PreparedStatement sql = conn.prepareStatement(sqlStatement)) {
+ int rowIndex = 0;
+ for (Map.Entry<String, String> keyValue : properties.entrySet()) {
+ sql.setString(rowIndex + 1, catalogName);
+ sql.setString(rowIndex + 2, namespaceName);
+ sql.setString(rowIndex + 3, keyValue.getKey());
+ sql.setString(rowIndex + 4, keyValue.getValue());
+ rowIndex += 4;
+ }
+ return sql.executeUpdate();
+ }
+ });
+
+ if (insertedRecords == properties.size()) {
+ LOG.debug("Successfully inserted {} properties for namespace {}",
properties, namespaceName);
+ return true;
+ } else {
+ throw new IllegalStateException();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedInterruptedException(e, "Interrupted in call to
insertProperties(namespace, properties) " +
+ "Namespace: %s", namespace);
+ } catch (SQLException e) {
+ throw new UncheckedSQLException(e, "Failed to insertProperties to
namespace: %s in catalog: %s", namespace,
+ catalogName);
+ }
+ }
+
+ private boolean updateProperties(Namespace namespace, Map<String, String>
properties) {
+ String namespaceName = JdbcUtil.namespaceToString(namespace);
+
+ try {
+ int updatedRecords = connections.run(conn -> {
+ String sqlStatement =
JdbcUtil.updatePropertiesStatement(properties.size());
+
+ try (PreparedStatement sql = conn.prepareStatement(sqlStatement)) {
+ int rowIndex = 0;
+ for (Map.Entry<String, String> keyValue : properties.entrySet()) {
+ sql.setString(rowIndex + 1, keyValue.getKey());
+ sql.setString(rowIndex + 2, keyValue.getValue());
+ rowIndex += 2;
+ }
+ sql.setString(rowIndex + 1, catalogName);
+ sql.setString(rowIndex + 2, namespaceName);
+ rowIndex += 2;
+ for (String key : properties.keySet()) {
+ sql.setString(rowIndex + 1, key);
+ rowIndex += 1;
+ }
+ LOG.info("Final log string {}", sql);
+ return sql.executeUpdate();
+ }
+ });
+
+ if (updatedRecords == properties.size()) {
+ LOG.debug("Successfully updated {} to new namespace: {}", properties,
namespaceName);
+ return true;
+ } else {
+ throw new IllegalStateException();
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedInterruptedException(e,
+ "Interrupted in call to updateProperties(namespace, properties)
Namespace: %s", namespace);
+ } catch (SQLException e) {
+ throw new UncheckedSQLException(e, "Failed to updateProperties to
namespace: %s in catalog: %s", namespace,
+ catalogName);
+ }
+ }
+
@Override
public void createNamespace(Namespace namespace, Map<String, String>
metadata) {
- throw new UnsupportedOperationException("Cannot create namespace " +
namespace +
- ": createNamespace is not supported");
+ if (namespaceExists(namespace)) {
+ throw new AlreadyExistsException("Namespace already exists: %s",
namespace);
+ }
+
+ if (metadata == null || metadata.isEmpty()) {
+ throw new IllegalArgumentException("Cannot create a namespace with null
or empty metadata");
+ }
Review comment:
After looking at the reasoning for disallowing empty metadata,
https://github.com/apache/iceberg/pull/3275/files#r741604136, should we add a
short comment above to indicate why this can't be empty?
Or, since we're using the metadata to indicate that a record exists, if
users don't supply metadata, should we simply place some sort of value in there
for them (possible choices I can think of might be a creation timestamp). This
way users would be able to create namespaces without explicitly passing in
metadata.
Returning to this PR after a while so forgive me if there's been a reason
covered that this is a bad idea already that I'm forgetting 😄
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -250,10 +265,93 @@ public void setConf(Configuration conf) {
this.conf = conf;
}
+ private boolean insertProperties(Namespace namespace, Map<String, String>
properties) {
+ String namespaceName = JdbcUtil.namespaceToString(namespace);
+
+ try {
+ int insertedRecords = connections.run(conn -> {
+ String sqlStatement = JdbcUtil.insertPropertiesStatement(properties);
+
+ try (PreparedStatement sql = conn.prepareStatement(sqlStatement)) {
+ int rowIndex = 0;
+ for (Map.Entry<String, String> keyValue : properties.entrySet()) {
+ sql.setString(++rowIndex, catalogName);
+ sql.setString(++rowIndex, namespaceName);
+ sql.setString(++rowIndex, keyValue.getKey());
+ sql.setString(++rowIndex, keyValue.getValue());
+ }
+ return sql.executeUpdate();
+ }
+ });
+
+ if (insertedRecords == properties.size()) {
+ LOG.debug("Successfully committed to new namespace: {}",
namespaceName);
+ return true;
+ } else {
+ throw new CommitFailedException("Failed to insertProperties to
namespace %s in catalog %s", namespaceName,
+ catalogName);
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new UncheckedInterruptedException(e,
+ "Interrupted in call to insertProperties(namespace, properties)
Namespace: %s", namespace);
+ } catch (SQLException e) {
+ throw new UncheckedSQLException(e, "Failed to insertProperties to
namespace: %s in catalog: %s", namespace,
+ catalogName);
+ }
+ }
+
+ private boolean updateProperties(Namespace namespace, Map<String, String>
properties) {
Review comment:
I would personally say if there's no practical difference between the
two, it would be ok to make them into one function.
Possibly there could be one outer function, that contains all of the shared
logic (like the interrupted exception and SQLException checks). And then two
different functions that wrap that function with a code block for the building
of the correct prepared statement. This might make the changes a bit easier to
digest and be cleaner, but it's fine as it is otherwise.
##########
File path: core/src/main/java/org/apache/iceberg/jdbc/JdbcCatalog.java
##########
@@ -340,14 +440,119 @@ public boolean dropNamespace(Namespace namespace) throws
NamespaceNotEmptyExcept
@Override
public boolean setProperties(Namespace namespace, Map<String, String>
properties) throws
NoSuchNamespaceException {
- throw new UnsupportedOperationException(
- "Cannot set properties " + namespace + " : setProperties is not
supported");
+ if (!namespaceExists(namespace)) {
+ throw new NoSuchNamespaceException("Namespace does not exist: %s",
namespace);
+ }
+
+ if (properties == null || properties.isEmpty()) {
+ throw new IllegalArgumentException("Cannot setProperties to a namespace
with null or empty properties");
+ }
Review comment:
Nit: This could probably be `Preconditions.checkArgument` or some
variant of that.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
