You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ywc88 (via GitHub)" <gi...@apache.org> on 2023/10/06 19:28:35 UTC
[PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
ywc88 opened a new pull request, #1179:
URL: https://github.com/apache/arrow-adbc/pull/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;
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#discussion_r1355044181
##########
c/validation/adbc_validation.cc:
##########
@@ -3425,6 +3425,64 @@ 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_EQ(3, rows_affected);
Review Comment:
You could assert that this is either `-1` or `3`.
##########
c/validation/adbc_validation.cc:
##########
@@ -3425,6 +3425,64 @@ 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_EQ(3, rows_affected);
+}
+
+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_EQ(-1, reader.rows_affected);
Review Comment:
I would probably assert that it's either `-1` (i.e. the driver does not know/the database did not report a value) or `5`, e.g.
```cpp
ASSERT_THAT(rows_affected,
::testing::AnyOf(::testing::Eq(5), ::testing::Eq(-1)));
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#issuecomment-1775744247
Is there any chance of getting this polished for 0.8.0? It would be helpful to have this implemented for our work integrating ADBC with DBI (which primarily uses the sqlite driver to test that all the wires are connected).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#issuecomment-1777619947
I can try to clean this up for the 0.8.0 milestone
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#discussion_r1372275043
##########
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)";
+ }
Review Comment:
Filed https://github.com/apache/arrow-adbc/issues/1226
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#issuecomment-1779448749
I can try too after I get a PR up for supporting dictionary-encoded strings + blobs.
While anybody is here and working on the value of `rows_affected`, I get an intermittent failing test related to that return value:
```
[ctest] [ RUN ] SqliteStatementTest.ResultInvalidation
[ctest] /Users/deweydunnington/Desktop/rscratch/arrow-adbc/c/validation/adbc_validation.cc:3691: Failure
[ctest] Value of: AdbcStatementExecuteQuery(&statement, &reader2.stream.value, &reader2.rows_affected, &error)
[ctest] Expected: is 0 (OK)
[ctest] Actual: '\t' (9) (of type unsigned char), 9 (INTERNAL)
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(c/driver/sqlite): Provide # of rows affected for non-SELECT statements instead of 0 [arrow-adbc]
Posted by "ywc88 (via GitHub)" <gi...@apache.org>.
ywc88 commented on PR #1179:
URL: https://github.com/apache/arrow-adbc/pull/1179#issuecomment-1779612208
Sorry for delaying on this, last week was offsite and Monday / Tuesday I was out recovering from a car accident.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org