From bdcef3869ec1b9b48f1264b8173dadfebae7ade5 Mon Sep 17 00:00:00 2001 From: Solomon Choe Date: Mon, 17 Jul 2023 13:59:56 -0700 Subject: [PATCH 1/4] fix(c/driver/sqlite): Wrap bulk ingests in a single begin/commit txn --- c/driver/sqlite/sqlite.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/c/driver/sqlite/sqlite.c b/c/driver/sqlite/sqlite.c index 6d0e38ea21..5521f99abc 100644 --- a/c/driver/sqlite/sqlite.c +++ b/c/driver/sqlite/sqlite.c @@ -1101,7 +1101,10 @@ AdbcStatusCode SqliteStatementExecuteIngest(struct SqliteStatement* stmt, AdbcStatusCode status = SqliteStatementInitIngest(stmt, &insert, error); int64_t row_count = 0; + int is_autocommit = sqlite3_get_autocommit(stmt->conn); if (status == ADBC_STATUS_OK) { + if (is_autocommit) sqlite3_exec(stmt->conn, "BEGIN TRANSACTION", 0, 0, 0); + while (1) { char finished = 0; status = @@ -1120,6 +1123,8 @@ AdbcStatusCode SqliteStatementExecuteIngest(struct SqliteStatement* stmt, } row_count++; } + + if (is_autocommit) sqlite3_exec(stmt->conn, "COMMIT", 0, 0, 0); } if (rows_affected) *rows_affected = row_count; From 85755de8c035c99150d874c775657076fe3219a1 Mon Sep 17 00:00:00 2001 From: Solomon Choe Date: Thu, 20 Jul 2023 11:41:30 -0700 Subject: [PATCH 2/4] Add a unit test for SqliteStatementExecuteIngest --- c/driver/sqlite/sqlite_test.cc | 42 +++++++++++++++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/c/driver/sqlite/sqlite_test.cc b/c/driver/sqlite/sqlite_test.cc index 3ab21fe3fa..0207c165cf 100644 --- a/c/driver/sqlite/sqlite_test.cc +++ b/c/driver/sqlite/sqlite_test.cc @@ -28,6 +28,7 @@ #include #include +#include "types.h" #include "statement_reader.h" #include "validation/adbc_validation.h" #include "validation/adbc_validation_util.h" @@ -639,6 +640,45 @@ TEST_F(SqliteReaderTest, MultiValueParams) { ASSERT_EQ(nullptr, reader.array->release); } +TEST_F(SqliteReaderTest, BulkIngestWithAutocommit) { + adbc_validation::StreamReader reader; + ASSERT_NO_FATAL_FAILURE(Exec("CREATE TABLE foo (col)")); + + // Filling out SqliteStatement explicitly to include `target_table`, which will + // make SqliteStatementExecuteQuery call SqliteStatementExecuteIngest + struct SqliteStatement sqlite_stmt; + std::memset(&sqlite_stmt, 0, sizeof(sqlite_stmt)); + + struct AdbcSqliteBinder binder; + std::memset(&binder, 0, sizeof(binder)); + sqlite_stmt.binder = binder; + + sqlite_stmt.conn = db; + sqlite_stmt.prepared = 0; + sqlite_stmt.append = 1; + sqlite_stmt.batch_size = 4; + + sqlite_stmt.target_table = strdup("foo"); + + struct AdbcStatement statement; + std::memset(&statement, 0, sizeof(statement)); + + struct AdbcConnection connection; + std::memset(&connection, 0, sizeof(connection)); + connection.private_data = db; + + AdbcStatusCode status = AdbcStatementNew(&connection, &statement, &error); + ASSERT_EQ(ADBC_STATUS_OK, status); + statement.private_data = &sqlite_stmt; + + std::string query = "INSERT INTO foo VALUES (1), (2), (3), (4)"; + status = AdbcStatementSetSqlQuery(&statement, query.c_str(), &error); + ASSERT_EQ(ADBC_STATUS_OK, status); + + ASSERT_EQ(ADBC_STATUS_OK, AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error)); + free(sqlite_stmt.target_table); +} + template class SqliteNumericParamTest : public SqliteReaderTest, public ::testing::WithParamInterface { @@ -696,4 +736,4 @@ INSTANTIATE_TEST_SUITE_P(FloatTypes, SqliteFloatParamTest, // floats (FLT_MIN/FLT_MAX isn't the right thing) // NANOARROW_TYPE_FLOAT, - NANOARROW_TYPE_DOUBLE)); + NANOARROW_TYPE_DOUBLE)); \ No newline at end of file From f8e8165e3cf404f71cfdbe33225327244f81668b Mon Sep 17 00:00:00 2001 From: Solomon Choe Date: Thu, 20 Jul 2023 12:55:20 -0700 Subject: [PATCH 3/4] Revert BulkIngestWithAutocommit; just going to benchmark this instead after seeing TestSqlIngestType --- c/driver/sqlite/sqlite_test.cc | 40 ---------------------------------- 1 file changed, 40 deletions(-) diff --git a/c/driver/sqlite/sqlite_test.cc b/c/driver/sqlite/sqlite_test.cc index 0207c165cf..e53bf5c29d 100644 --- a/c/driver/sqlite/sqlite_test.cc +++ b/c/driver/sqlite/sqlite_test.cc @@ -28,7 +28,6 @@ #include #include -#include "types.h" #include "statement_reader.h" #include "validation/adbc_validation.h" #include "validation/adbc_validation_util.h" @@ -640,45 +639,6 @@ TEST_F(SqliteReaderTest, MultiValueParams) { ASSERT_EQ(nullptr, reader.array->release); } -TEST_F(SqliteReaderTest, BulkIngestWithAutocommit) { - adbc_validation::StreamReader reader; - ASSERT_NO_FATAL_FAILURE(Exec("CREATE TABLE foo (col)")); - - // Filling out SqliteStatement explicitly to include `target_table`, which will - // make SqliteStatementExecuteQuery call SqliteStatementExecuteIngest - struct SqliteStatement sqlite_stmt; - std::memset(&sqlite_stmt, 0, sizeof(sqlite_stmt)); - - struct AdbcSqliteBinder binder; - std::memset(&binder, 0, sizeof(binder)); - sqlite_stmt.binder = binder; - - sqlite_stmt.conn = db; - sqlite_stmt.prepared = 0; - sqlite_stmt.append = 1; - sqlite_stmt.batch_size = 4; - - sqlite_stmt.target_table = strdup("foo"); - - struct AdbcStatement statement; - std::memset(&statement, 0, sizeof(statement)); - - struct AdbcConnection connection; - std::memset(&connection, 0, sizeof(connection)); - connection.private_data = db; - - AdbcStatusCode status = AdbcStatementNew(&connection, &statement, &error); - ASSERT_EQ(ADBC_STATUS_OK, status); - statement.private_data = &sqlite_stmt; - - std::string query = "INSERT INTO foo VALUES (1), (2), (3), (4)"; - status = AdbcStatementSetSqlQuery(&statement, query.c_str(), &error); - ASSERT_EQ(ADBC_STATUS_OK, status); - - ASSERT_EQ(ADBC_STATUS_OK, AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error)); - free(sqlite_stmt.target_table); -} - template class SqliteNumericParamTest : public SqliteReaderTest, public ::testing::WithParamInterface { From 837354cf540e210d5699f25f620460b7f385136e Mon Sep 17 00:00:00 2001 From: Solomon Choe Date: Mon, 24 Jul 2023 15:26:54 -0700 Subject: [PATCH 4/4] [lint] Add new line --- c/driver/sqlite/sqlite_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/driver/sqlite/sqlite_test.cc b/c/driver/sqlite/sqlite_test.cc index e53bf5c29d..3ab21fe3fa 100644 --- a/c/driver/sqlite/sqlite_test.cc +++ b/c/driver/sqlite/sqlite_test.cc @@ -696,4 +696,4 @@ INSTANTIATE_TEST_SUITE_P(FloatTypes, SqliteFloatParamTest, // floats (FLT_MIN/FLT_MAX isn't the right thing) // NANOARROW_TYPE_FLOAT, - NANOARROW_TYPE_DOUBLE)); \ No newline at end of file + NANOARROW_TYPE_DOUBLE));