nastra commented on code in PR #6952:
URL: https://github.com/apache/iceberg/pull/6952#discussion_r1122838263
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/JdbcSnowflakeClient.java:
##########
@@ -125,6 +129,13 @@ public <T> T query(Connection conn, String sql,
ResultSetParser<T> parser, Strin
private final JdbcClientPool connectionPool;
private QueryHarness queryHarness;
+ protected static final Set<Integer> DATABASE_NOT_FOUND_ERROR_CODES =
Review Comment:
It seems these are only used in tests, so should all of these maybe be
package-private and have a `@VisibleForTesting` annotation?
##########
snowflake/src/test/java/org/apache/iceberg/snowflake/JdbcSnowflakeClientTest.java:
##########
@@ -367,15 +535,66 @@ public void testListIcebergTablesInSchema() throws
SQLException {
/**
* Any unexpected SQLException from the underlying connection will propagate
out as an
- * UncheckedSQLException when listing tables.
+ * UncheckedSQLException when listing tables at Root level
*/
@Test
- public void testListIcebergTablesSQLException() throws SQLException,
InterruptedException {
- when(mockClientPool.run(any(ClientPool.Action.class)))
- .thenThrow(new SQLException("Fake SQL exception"));
- Assertions.assertThatExceptionOfType(UncheckedSQLException.class)
- .isThrownBy(() ->
snowflakeClient.listIcebergTables(SnowflakeIdentifier.ofDatabase("DB_1")))
- .withStackTraceContaining("Fake SQL exception");
+ public void testListIcebergTablesSQLExceptionAtRootLevel()
+ throws SQLException, InterruptedException {
+ generateExceptionAndAssert(
+ () -> snowflakeClient.listIcebergTables(SnowflakeIdentifier.ofRoot()),
+ new SQLException(String.format("SQL exception with Error Code %d", 0),
"2000", 0, null),
+ UncheckedSQLException.class);
+ }
+
+ /**
+ * Any unexpected SQLException with specific error codes from the underlying
connection will
+ * propagate out as a NoSuchNamespaceException when listing tables at
Database level
+ */
+ @Test
+ public void testListIcebergTablesSQLExceptionAtDatabaseLevel()
+ throws SQLException, InterruptedException {
+ for (Integer errorCode : DATABASE_NOT_FOUND_ERROR_CODES) {
+ generateExceptionAndAssert(
+ () ->
snowflakeClient.listIcebergTables(SnowflakeIdentifier.ofDatabase("DB_1")),
+ new SQLException(
+ String.format("SQL exception with Error Code %d", errorCode),
+ "2000",
+ errorCode,
+ null),
+ NoSuchNamespaceException.class);
+ }
+ }
+
+ /**
+ * Any unexpected SQLException with specific error codes from the underlying
connection will
+ * propagate out as a NoSuchNamespaceException when listing tables at Schema
level
+ */
+ @Test
+ public void testListIcebergTablesSQLExceptionAtSchemaLevel()
+ throws SQLException, InterruptedException {
+ for (Integer errorCode : SCHEMA_NOT_FOUND_ERROR_CODES) {
Review Comment:
TBH using `generateExceptionAndAssert()` makes the code quite difficult to
read and reason about. I understand the argument for reusing the same code, but
I think tests are clearer to read when we're asserting everything in the test
method, similar to below:
```
@Test
public void testListIcebergTablesSQLExceptionAtSchemaLevel()
throws SQLException, InterruptedException {
for (Integer errorCode : SCHEMA_NOT_FOUND_ERROR_CODES) {
SQLException injectedException =
new SQLException(
String.format("SQL exception with Error Code %d", errorCode),
"2000",
errorCode,
null);
when(mockClientPool.run(any(ClientPool.Action.class))).thenThrow(injectedException);
Assertions.assertThatExceptionOfType(NoSuchNamespaceException.class)
.isThrownBy(
() ->
snowflakeClient.listIcebergTables(
SnowflakeIdentifier.ofSchema("DB_1", "SCHEMA_1")))
.withMessageContaining("Identifier not found: 'SCHEMA:
'DB_1.SCHEMA_1''")
.withCause(injectedException);
}
}
```
Note that we're using `.withMessageContaining` to make sure we're getting
the right error message, and `.withCause` allows to make sure we get the
correct exception in the cause.
##########
snowflake/src/test/java/org/apache/iceberg/snowflake/JdbcSnowflakeClientTest.java:
##########
@@ -520,14 +757,12 @@ public void testGetTableMetadataSQLException() throws
SQLException, InterruptedE
*/
@Test
public void testGetTableMetadataInterruptedException() throws SQLException,
InterruptedException {
- when(mockClientPool.run(any(ClientPool.Action.class)))
Review Comment:
after reading through the entire changes in this class I don't think we
should be introducing `generateExceptionAndAssert` as this makes it more
difficult to read the code
##########
snowflake/src/main/java/org/apache/iceberg/snowflake/JdbcSnowflakeClient.java:
##########
@@ -208,6 +219,7 @@ public List<SnowflakeIdentifier> listDatabases() {
queryHarness.query(
conn, "SHOW DATABASES IN ACCOUNT",
DATABASE_RESULT_SET_HANDLER));
} catch (SQLException e) {
+ tryMapSnowflakeExceptionToIcebergException(SnowflakeIdentifier.ofRoot(),
e);
Review Comment:
reading through the catch clause here and seeing this, one wouldn't expect
`tryMapSnowflakeExceptionToIcebergException` to actually throw. I think it
would be good to rewrite the code so that it's more obvious.
What about having `snowflakeExceptionToIcebergException` return an
`Optional<RuntimeException>`? Then we could simply have `throw
snowflakeExceptionToIcebergException(SnowflakeIdentifier.ofRoot(),
e).orElseGet(() -> new UncheckedSQLException(e, "Failed to list databases"));`
here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [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]
