dannycjones commented on code in PR #3062:
URL: https://github.com/apache/iceberg-rust/pull/3062#discussion_r3855673913
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1386,6 +1389,70 @@ mod tests {
new_sql_catalog(warehouse_loc.clone(), Some("iceberg")).await;
}
+ #[tokio::test]
+ async fn test_execute_returns_commit_error() {
+ let sql_lite_uri = format!("sqlite:{}", temp_path());
+ sqlx::Sqlite::create_database(&sql_lite_uri).await.unwrap();
+ let catalog = SqlCatalogBuilder::default()
+ .with_storage_factory(Arc::new(LocalFsStorageFactory))
+ .prop("pool.max-connections", "1")
+ .load(
+ "iceberg",
+ HashMap::from_iter([
+ (SQL_CATALOG_PROP_URI.to_string(), sql_lite_uri),
+ (SQL_CATALOG_PROP_WAREHOUSE.to_string(), temp_path()),
+ ]),
+ )
+ .await
+ .unwrap();
+
+ catalog
+ .connection
+ .execute("PRAGMA foreign_keys = ON")
+ .await
+ .unwrap();
+ catalog
+ .connection
+ .execute("CREATE TABLE parent(id INTEGER PRIMARY KEY)")
+ .await
+ .unwrap();
+ catalog
+ .connection
+ .execute(
+ "CREATE TABLE child(parent_id INTEGER REFERENCES parent(id) \
+ DEFERRABLE INITIALLY DEFERRED)",
+ )
+ .await
+ .unwrap();
+
+ assert!(
+ catalog
+ .execute("INSERT INTO child VALUES (1)", vec![], None)
+ .await
+ .is_err()
+ );
+ let child_count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM child")
+ .fetch_one(&catalog.connection)
+ .await
+ .unwrap();
+ assert_eq!(child_count, 0);
+
+ catalog
+ .connection
+ .execute("INSERT INTO parent VALUES (1)")
+ .await
+ .unwrap();
+ catalog
+ .execute("INSERT INTO child VALUES (1)", vec![], None)
+ .await
+ .unwrap();
+ let child_count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM child")
+ .fetch_one(&catalog.connection)
+ .await
+ .unwrap();
+ assert_eq!(child_count, 1);
+ }
Review Comment:
Would it be possible to extend this test to also cover the scenario where
the transaction is passed in outside of `execute`?
##########
crates/catalog/sql/src/catalog.rs:
##########
@@ -1386,6 +1389,70 @@ mod tests {
new_sql_catalog(warehouse_loc.clone(), Some("iceberg")).await;
}
+ #[tokio::test]
+ async fn test_execute_returns_commit_error() {
+ let sql_lite_uri = format!("sqlite:{}", temp_path());
+ sqlx::Sqlite::create_database(&sql_lite_uri).await.unwrap();
+ let catalog = SqlCatalogBuilder::default()
+ .with_storage_factory(Arc::new(LocalFsStorageFactory))
+ .prop("pool.max-connections", "1")
+ .load(
+ "iceberg",
+ HashMap::from_iter([
+ (SQL_CATALOG_PROP_URI.to_string(), sql_lite_uri),
+ (SQL_CATALOG_PROP_WAREHOUSE.to_string(), temp_path()),
+ ]),
+ )
+ .await
+ .unwrap();
+
+ catalog
+ .connection
+ .execute("PRAGMA foreign_keys = ON")
+ .await
+ .unwrap();
+ catalog
+ .connection
+ .execute("CREATE TABLE parent(id INTEGER PRIMARY KEY)")
+ .await
+ .unwrap();
+ catalog
+ .connection
+ .execute(
+ "CREATE TABLE child(parent_id INTEGER REFERENCES parent(id) \
+ DEFERRABLE INITIALLY DEFERRED)",
+ )
+ .await
+ .unwrap();
+
+ assert!(
+ catalog
+ .execute("INSERT INTO child VALUES (1)", vec![], None)
+ .await
+ .is_err()
+ );
+ let child_count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM child")
+ .fetch_one(&catalog.connection)
+ .await
+ .unwrap();
+ assert_eq!(child_count, 0);
+
+ catalog
+ .connection
+ .execute("INSERT INTO parent VALUES (1)")
+ .await
+ .unwrap();
+ catalog
+ .execute("INSERT INTO child VALUES (1)", vec![], None)
+ .await
+ .unwrap();
+ let child_count: i64 = sqlx::query_scalar("SELECT COUNT(*) FROM child")
+ .fetch_one(&catalog.connection)
+ .await
+ .unwrap();
+ assert_eq!(child_count, 1);
+ }
Review Comment:
I think this test needs a narrative comment to explain why we're writing
some complex SQL unrelated to Iceberg. It's a bit weird, although I don't know
a better way to exercise the `execute` error handling.
Something to this effect would help:
```rust
// Create a table that will reliably fail at database transaction commit.
// Table has a foreign key relationship, where child column reference parent
ID field.
// This allows to exercise error handling where part of the table operation
succeeds, but the transaction itself fails.
```
I think if we structure it a bit, it'll come clear what exactly we're
testing.
--
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]
