You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by li...@apache.org on 2023/10/25 20:11:29 UTC
[arrow-adbc] branch main updated: fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 (#1179)
This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new 7d337684 fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 (#1179)
7d337684 is described below
commit 7d33768489c0b6ba243b77003de888062b49a5e0
Author: Solomon Choe <12...@users.noreply.github.com>
AuthorDate: Wed Oct 25 13:11:23 2023 -0700
fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 (#1179)
Fixes #1145
If we prefer consistency with the stream case for # of rows affected
instead of the actual # of rows affected, we can change this line:
```
*rows_affected = sqlite3_changes(stmt->conn);
```
to be:
```
*rows_affected = -1;
```
---
c/driver/flightsql/dremio_flightsql_test.cc | 6 +++
c/driver/flightsql/sqlite_flightsql_test.cc | 6 +++
c/driver/postgresql/postgresql_test.cc | 6 +++
c/driver/sqlite/sqlite.c | 8 +++-
c/integration/duckdb/duckdb_test.cc | 6 +++
c/validation/adbc_validation.cc | 60 +++++++++++++++++++++++++++++
c/validation/adbc_validation.h | 6 +++
7 files changed, 97 insertions(+), 1 deletion(-)
diff --git a/c/driver/flightsql/dremio_flightsql_test.cc b/c/driver/flightsql/dremio_flightsql_test.cc
index 52c184f3..8c59eb4a 100644
--- a/c/driver/flightsql/dremio_flightsql_test.cc
+++ b/c/driver/flightsql/dremio_flightsql_test.cc
@@ -92,6 +92,12 @@ class DremioFlightSqlStatementTest : public ::testing::Test,
void TestSqlIngestColumnEscaping() {
GTEST_SKIP() << "Column escaping not implemented";
}
+ void TestSqlQueryRowsAffectedDelete() {
+ GTEST_SKIP() << "Cannot query rows affected in delete (not implemented)";
+ }
+ void TestSqlQueryRowsAffectedDeleteStream() {
+ GTEST_SKIP() << "Cannot query rows affected in delete stream (not implemented)";
+ }
protected:
DremioFlightSqlQuirks quirks_;
diff --git a/c/driver/flightsql/sqlite_flightsql_test.cc b/c/driver/flightsql/sqlite_flightsql_test.cc
index 41a7dedf..a736d253 100644
--- a/c/driver/flightsql/sqlite_flightsql_test.cc
+++ b/c/driver/flightsql/sqlite_flightsql_test.cc
@@ -274,6 +274,12 @@ class SqliteFlightSqlStatementTest : public ::testing::Test,
void TestSqlIngestInterval() {
GTEST_SKIP() << "Cannot ingest Interval (not implemented)";
}
+ void TestSqlQueryRowsAffectedDelete() {
+ GTEST_SKIP() << "Cannot query rows affected in delete (not implemented)";
+ }
+ void TestSqlQueryRowsAffectedDeleteStream() {
+ GTEST_SKIP() << "Cannot query rows affected in delete stream (not implemented)";
+ }
protected:
SqliteFlightSqlQuirks quirks_;
diff --git a/c/driver/postgresql/postgresql_test.cc b/c/driver/postgresql/postgresql_test.cc
index 932e685e..d762ef5b 100644
--- a/c/driver/postgresql/postgresql_test.cc
+++ b/c/driver/postgresql/postgresql_test.cc
@@ -816,6 +816,12 @@ class PostgresStatementTest : public ::testing::Test,
void TestSqlPrepareErrorParamCountMismatch() { GTEST_SKIP() << "Not yet implemented"; }
void TestSqlPrepareGetParameterSchema() { GTEST_SKIP() << "Not yet implemented"; }
void TestSqlPrepareSelectParams() { GTEST_SKIP() << "Not yet implemented"; }
+ void TestSqlQueryRowsAffectedDelete() {
+ GTEST_SKIP() << "Cannot query rows affected in delete (not implemented)";
+ }
+ void TestSqlQueryRowsAffectedDeleteStream() {
+ GTEST_SKIP() << "Cannot query rows affected in delete stream (not implemented)";
+ }
void TestConcurrentStatements() {
// TODO: refactor driver so that we read all the data as soon as
diff --git a/c/driver/sqlite/sqlite.c b/c/driver/sqlite/sqlite.c
index 05243c03..e4928018 100644
--- a/c/driver/sqlite/sqlite.c
+++ b/c/driver/sqlite/sqlite.c
@@ -1364,7 +1364,13 @@ AdbcStatusCode SqliteStatementExecuteQuery(struct AdbcStatement* statement,
sqlite3_mutex_leave(sqlite3_db_mutex(stmt->conn));
AdbcSqliteBinderRelease(&stmt->binder);
- if (rows_affected) *rows_affected = rows;
+ if (rows_affected) {
+ if (sqlite3_column_count(stmt->stmt) == 0) {
+ *rows_affected = sqlite3_changes(stmt->conn);
+ } else {
+ *rows_affected = rows;
+ }
+ }
return status;
}
diff --git a/c/integration/duckdb/duckdb_test.cc b/c/integration/duckdb/duckdb_test.cc
index a6bded03..37bb03e5 100644
--- a/c/integration/duckdb/duckdb_test.cc
+++ b/c/integration/duckdb/duckdb_test.cc
@@ -101,6 +101,12 @@ class DuckDbStatementTest : public ::testing::Test,
}
void TestSqlQueryErrors() { GTEST_SKIP() << "DuckDB does not set AdbcError.release"; }
+ void TestSqlQueryRowsAffectedDelete() {
+ GTEST_SKIP() << "Cannot query rows affected in delete (not implemented)";
+ }
+ void TestSqlQueryRowsAffectedDeleteStream() {
+ GTEST_SKIP() << "Cannot query rows affected in delete stream (not implemented)";
+ }
void TestErrorCompatibility() {
GTEST_SKIP() << "DuckDB does not set AdbcError.release";
diff --git a/c/validation/adbc_validation.cc b/c/validation/adbc_validation.cc
index c2f32ff5..6dd3fb7b 100644
--- a/c/validation/adbc_validation.cc
+++ b/c/validation/adbc_validation.cc
@@ -3434,6 +3434,66 @@ void StatementTest::TestSqlQueryTrailingSemicolons() {
ASSERT_THAT(AdbcStatementRelease(&statement, &error), IsOkStatus(&error));
}
+void StatementTest::TestSqlQueryRowsAffectedDelete() {
+ ASSERT_THAT(quirks()->DropTable(&connection, "delete_test", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "CREATE TABLE delete_test (foo INT)", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
+ IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "INSERT INTO delete_test (foo) VALUES (1), (2), (3), (4), (5)", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
+ IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "DELETE FROM delete_test WHERE foo >= 3", &error),
+ IsOkStatus(&error));
+
+ int64_t rows_affected = 0;
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, &rows_affected, &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(rows_affected,
+ ::testing::AnyOf(::testing::Eq(3), ::testing::Eq(-1)));
+}
+
+void StatementTest::TestSqlQueryRowsAffectedDeleteStream() {
+ ASSERT_THAT(quirks()->DropTable(&connection, "delete_test", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementNew(&connection, &statement, &error), IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "CREATE TABLE delete_test (foo INT)", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
+ IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "INSERT INTO delete_test (foo) VALUES (1), (2), (3), (4), (5)", &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, nullptr, nullptr, &error),
+ IsOkStatus(&error));
+
+ ASSERT_THAT(AdbcStatementSetSqlQuery(&statement,
+ "DELETE FROM delete_test WHERE foo >= 3", &error),
+ IsOkStatus(&error));
+
+ adbc_validation::StreamReader reader;
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement, &reader.stream.value,
+ &reader.rows_affected, &error),
+ IsOkStatus(&error));
+ ASSERT_THAT(reader.rows_affected,
+ ::testing::AnyOf(::testing::Eq(5), ::testing::Eq(-1)));
+}
+
+
+
+
void StatementTest::TestTransactions() {
if (!quirks()->supports_transactions() || quirks()->ddl_implicit_commit_txn()) {
GTEST_SKIP();
diff --git a/c/validation/adbc_validation.h b/c/validation/adbc_validation.h
index d125d1be..2e4c894d 100644
--- a/c/validation/adbc_validation.h
+++ b/c/validation/adbc_validation.h
@@ -365,6 +365,8 @@ class StatementTest {
void TestSqlQueryCancel();
void TestSqlQueryErrors();
void TestSqlQueryTrailingSemicolons();
+ void TestSqlQueryRowsAffectedDelete();
+ void TestSqlQueryRowsAffectedDeleteStream();
void TestSqlSchemaInts();
void TestSqlSchemaFloats();
@@ -455,6 +457,10 @@ class StatementTest {
TEST_F(FIXTURE, SqlQueryCancel) { TestSqlQueryCancel(); } \
TEST_F(FIXTURE, SqlQueryErrors) { TestSqlQueryErrors(); } \
TEST_F(FIXTURE, SqlQueryTrailingSemicolons) { TestSqlQueryTrailingSemicolons(); } \
+ TEST_F(FIXTURE, SqlQueryRowsAffectedDelete) { TestSqlQueryRowsAffectedDelete(); } \
+ TEST_F(FIXTURE, SqlQueryRowsAffectedDeleteStream) { \
+ TestSqlQueryRowsAffectedDeleteStream(); \
+ } \
TEST_F(FIXTURE, SqlSchemaInts) { TestSqlSchemaInts(); } \
TEST_F(FIXTURE, SqlSchemaFloats) { TestSqlSchemaFloats(); } \
TEST_F(FIXTURE, SqlSchemaStrings) { TestSqlSchemaStrings(); } \