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 1a07f244 fix(c/driver/sqlite): fix escaping of sqlite TABLE CREATE
columns (#906)
1a07f244 is described below
commit 1a07f244b6ef4a65067550f36e51142b937be37b
Author: William Ayd <[email protected]>
AuthorDate: Mon Jul 17 10:35:03 2023 -0700
fix(c/driver/sqlite): fix escaping of sqlite TABLE CREATE columns (#906)
Should maybe use sqlite3's string handling throughout the module; for
now just focused on CREATE TABLE
fixes
https://github.com/apache/arrow-adbc/issues/905#issuecomment-1636418672
---
c/driver/flightsql/dremio_flightsql_test.cc | 2 ++
c/driver/flightsql/sqlite_flightsql_test.cc | 2 ++
c/driver/sqlite/sqlite.c | 52 +++++++++++++++++------------
c/integration/duckdb/duckdb_test.cc | 2 ++
c/validation/adbc_validation.cc | 25 ++++++++++++++
c/validation/adbc_validation.h | 2 ++
6 files changed, 64 insertions(+), 21 deletions(-)
diff --git a/c/driver/flightsql/dremio_flightsql_test.cc
b/c/driver/flightsql/dremio_flightsql_test.cc
index f4e0642c..c128bd49 100644
--- a/c/driver/flightsql/dremio_flightsql_test.cc
+++ b/c/driver/flightsql/dremio_flightsql_test.cc
@@ -87,6 +87,8 @@ class DremioFlightSqlStatementTest : public ::testing::Test,
void SetUp() override { ASSERT_NO_FATAL_FAILURE(SetUpTest()); }
void TearDown() override { ASSERT_NO_FATAL_FAILURE(TearDownTest()); }
+ void TestSqlIngestTableEscaping() { GTEST_SKIP() << "Table escaping not
implemented"; }
+
protected:
DremioFlightSqlQuirks quirks_;
};
diff --git a/c/driver/flightsql/sqlite_flightsql_test.cc
b/c/driver/flightsql/sqlite_flightsql_test.cc
index 96d448a0..fdb2a795 100644
--- a/c/driver/flightsql/sqlite_flightsql_test.cc
+++ b/c/driver/flightsql/sqlite_flightsql_test.cc
@@ -228,6 +228,8 @@ class SqliteFlightSqlStatementTest : public ::testing::Test,
void SetUp() override { ASSERT_NO_FATAL_FAILURE(SetUpTest()); }
void TearDown() override { ASSERT_NO_FATAL_FAILURE(TearDownTest()); }
+ void TestSqlIngestTableEscaping() { GTEST_SKIP() << "Table escaping not
implemented"; }
+
protected:
SqliteFlightSqlQuirks quirks_;
};
diff --git a/c/driver/sqlite/sqlite.c b/c/driver/sqlite/sqlite.c
index 4124098f..6d0e38ea 100644
--- a/c/driver/sqlite/sqlite.c
+++ b/c/driver/sqlite/sqlite.c
@@ -981,23 +981,22 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
AdbcStatusCode code = ADBC_STATUS_OK;
// Create statements for CREATE TABLE / INSERT
- struct StringBuilder create_query = {0};
- struct StringBuilder insert_query = {0};
-
- if (StringBuilderInit(&create_query, /*initial_size=*/256) != 0) {
- SetError(error, "[SQLite] Could not initiate StringBuilder");
+ sqlite3_str* create_query = sqlite3_str_new(NULL);
+ if (sqlite3_str_errcode(create_query)) {
+ SetError(error, "[SQLite] %s", sqlite3_errmsg(stmt->conn));
return ADBC_STATUS_INTERNAL;
}
+ struct StringBuilder insert_query = {0};
if (StringBuilderInit(&insert_query, /*initial_size=*/256) != 0) {
SetError(error, "[SQLite] Could not initiate StringBuilder");
- StringBuilderReset(&create_query);
+ sqlite3_free(sqlite3_str_finish(create_query));
return ADBC_STATUS_INTERNAL;
}
- if (StringBuilderAppend(&create_query, "%s%s%s", "CREATE TABLE ",
stmt->target_table,
- " (") != 0) {
- SetError(error, "[SQLite] Call to StringBuilderAppend failed");
+ sqlite3_str_appendf(create_query, "%s%Q%s", "CREATE TABLE ",
stmt->target_table, " (");
+ if (sqlite3_str_errcode(create_query)) {
+ SetError(error, "[SQLite] %s", sqlite3_errmsg(stmt->conn));
code = ADBC_STATUS_INTERNAL;
goto cleanup;
}
@@ -1010,11 +1009,18 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
}
for (int i = 0; i < stmt->binder.schema.n_children; i++) {
- if (i > 0) StringBuilderAppend(&create_query, "%s", ", ");
- // XXX: should escape the column name too
- if (StringBuilderAppend(&create_query, "%s",
stmt->binder.schema.children[i]->name) !=
- 0) {
- SetError(error, "[SQLite] Call to StringBuilderAppend failed");
+ if (i > 0) {
+ sqlite3_str_appendf(create_query, "%s", ", ");
+ if (sqlite3_str_errcode(create_query)) {
+ SetError(error, "[SQLite] %s", sqlite3_errmsg(stmt->conn));
+ code = ADBC_STATUS_INTERNAL;
+ goto cleanup;
+ }
+ }
+
+ sqlite3_str_appendf(create_query, "%Q",
stmt->binder.schema.children[i]->name);
+ if (sqlite3_str_errcode(create_query)) {
+ SetError(error, "[SQLite] %s", sqlite3_errmsg(stmt->conn));
code = ADBC_STATUS_INTERNAL;
goto cleanup;
}
@@ -1033,8 +1039,10 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
goto cleanup;
}
}
- if (StringBuilderAppend(&create_query, "%s", ")") != 0) {
- SetError(error, "[SQLite] Call to StringBuilderAppend failed");
+
+ sqlite3_str_appendchar(create_query, 1, ')');
+ if (sqlite3_str_errcode(create_query)) {
+ SetError(error, "[SQLite] %s", sqlite3_errmsg(stmt->conn));
code = ADBC_STATUS_INTERNAL;
goto cleanup;
}
@@ -1048,15 +1056,17 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
sqlite3_stmt* create = NULL;
if (!stmt->append) {
// Create table
- int rc = sqlite3_prepare_v2(stmt->conn, create_query.buffer,
(int)create_query.size,
- &create, /*pzTail=*/NULL);
+ int rc =
+ sqlite3_prepare_v2(stmt->conn, sqlite3_str_value(create_query),
+ sqlite3_str_length(create_query), &create,
/*pzTail=*/NULL);
if (rc == SQLITE_OK) {
rc = sqlite3_step(create);
}
if (rc != SQLITE_OK && rc != SQLITE_DONE) {
- SetError(error, "[SQLite] Failed to create table: %s (executed '%s')",
- sqlite3_errmsg(stmt->conn), create_query.buffer);
+ SetError(error, "[SQLite] Failed to create table: %s (executed '%.*s')",
+ sqlite3_errmsg(stmt->conn), sqlite3_str_length(create_query),
+ sqlite3_str_value(create_query));
code = ADBC_STATUS_INTERNAL;
}
}
@@ -1074,7 +1084,7 @@ AdbcStatusCode SqliteStatementInitIngest(struct
SqliteStatement* stmt,
sqlite3_finalize(create);
cleanup:
- StringBuilderReset(&create_query);
+ sqlite3_free(sqlite3_str_finish(create_query));
StringBuilderReset(&insert_query);
return code;
}
diff --git a/c/integration/duckdb/duckdb_test.cc
b/c/integration/duckdb/duckdb_test.cc
index b2a5c524..fd6e1984 100644
--- a/c/integration/duckdb/duckdb_test.cc
+++ b/c/integration/duckdb/duckdb_test.cc
@@ -94,6 +94,8 @@ class DuckDbStatementTest : public ::testing::Test,
// Accepts Prepare() without any query
void TestSqlPrepareErrorNoQuery() { GTEST_SKIP(); }
+ void TestSqlIngestTableEscaping() { GTEST_SKIP() << "Table escaping not
implemented"; }
+
protected:
DuckDbQuirks quirks_;
};
diff --git a/c/validation/adbc_validation.cc b/c/validation/adbc_validation.cc
index bba51588..208b3f3b 100644
--- a/c/validation/adbc_validation.cc
+++ b/c/validation/adbc_validation.cc
@@ -1193,6 +1193,31 @@ void StatementTest::TestSqlIngestTimestampTz() {
TestSqlIngestTemporalType<NANOARROW_TIME_UNIT_NANO>("America/Los_Angeles"));
}
+void StatementTest::TestSqlIngestTableEscaping() {
+ std::string name = "create_table_escaping";
+
+ ASSERT_THAT(quirks()->DropTable(&connection, name, &error),
+ adbc_validation::IsOkStatus(&error));
+ Handle<struct ArrowSchema> schema;
+ Handle<struct ArrowArray> array;
+ struct ArrowError na_error;
+ ASSERT_THAT(MakeSchema(&schema.value, {{"index", NANOARROW_TYPE_INT64}}),
IsOkErrno());
+ ASSERT_THAT((MakeBatch<int64_t>(&schema.value, &array.value, &na_error,
+ {42, -42, std::nullopt})),
+ IsOkErrno());
+
+ Handle<struct AdbcStatement> statement;
+ ASSERT_THAT(AdbcStatementNew(&connection, &statement.value, &error),
IsOkErrno());
+ ASSERT_THAT(AdbcStatementSetOption(&statement.value,
ADBC_INGEST_OPTION_TARGET_TABLE,
+ name.c_str(), &error),
+ IsOkErrno());
+ ASSERT_THAT(AdbcStatementBind(&statement.value, &array.value, &schema.value,
&error),
+ IsOkErrno());
+ ASSERT_THAT(AdbcStatementExecuteQuery(&statement.value, nullptr, nullptr,
&error),
+ IsOkErrno());
+ ASSERT_THAT(AdbcStatementRelease(&statement.value, &error), IsOkErrno());
+}
+
void StatementTest::TestSqlIngestAppend() {
if (!quirks()->supports_bulk_ingest()) {
GTEST_SKIP();
diff --git a/c/validation/adbc_validation.h b/c/validation/adbc_validation.h
index aeafa9c3..61208d13 100644
--- a/c/validation/adbc_validation.h
+++ b/c/validation/adbc_validation.h
@@ -235,6 +235,7 @@ class StatementTest {
// ---- End Type-specific tests ----------------
+ void TestSqlIngestTableEscaping();
void TestSqlIngestAppend();
void TestSqlIngestErrors();
void TestSqlIngestMultipleConnections();
@@ -301,6 +302,7 @@ class StatementTest {
TEST_F(FIXTURE, SqlIngestBinary) { TestSqlIngestBinary(); }
\
TEST_F(FIXTURE, SqlIngestTimestamp) { TestSqlIngestTimestamp(); }
\
TEST_F(FIXTURE, SqlIngestTimestampTz) { TestSqlIngestTimestampTz(); }
\
+ TEST_F(FIXTURE, SqlIngestTableEscaping) { TestSqlIngestTableEscaping(); }
\
TEST_F(FIXTURE, SqlIngestAppend) { TestSqlIngestAppend(); }
\
TEST_F(FIXTURE, SqlIngestErrors) { TestSqlIngestErrors(); }
\
TEST_F(FIXTURE, SqlIngestMultipleConnections) {
TestSqlIngestMultipleConnections(); } \
