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 10ccaab781 fix(c/driver/sqlite): bring driver up to par, enable
validation (#4845)
10ccaab781 is described below
commit 10ccaab781d92b6551d52087e36d1d8e9651bb5e
Author: David Li <[email protected]>
AuthorDate: Thu Oct 1 17:41:47 2026 -0700
fix(c/driver/sqlite): bring driver up to par, enable validation (#4845)
- ConnectionGetOption: return NOT_FOUND by default
- ConnectionGetInfo: return the driver version
- ConnectionGetObjects: return not-null info properly
- Allow setting target ingest schema so long as it's empty
- Return ALREADY_EXISTS for ingest operations where possible
---
.github/workflows/integration.yml | 55 ++++++++++++++++++++++
c/driver/framework/connection.h | 3 +-
c/driver/sqlite/CMakeLists.txt | 9 +++-
.../queries/ingest/string.txtcase => config.h.in} | 5 +-
c/driver/sqlite/meson.build | 6 +++
c/driver/sqlite/sqlite.cc | 38 ++++++++++-----
c/driver/sqlite/sqlite_test.cc | 9 ++--
.../validation/queries/ingest/string.txtcase | 10 ++++
c/driver/sqlite/validation/tests/sqlite.py | 2 +-
c/driver_manager/adbc_driver_manager_test.cc | 3 +-
r/adbcsqlite/bootstrap.R | 8 ++++
r/tools/bootstrap-c.R | 2 +-
ruby/test/test-connection.rb | 7 +--
13 files changed, 129 insertions(+), 28 deletions(-)
diff --git a/.github/workflows/integration.yml
b/.github/workflows/integration.yml
index c0e4a76ef7..509c589347 100644
--- a/.github/workflows/integration.yml
+++ b/.github/workflows/integration.yml
@@ -174,6 +174,61 @@ jobs:
run: |
docker compose down
+ sqlite:
+ name: "SQLite Validation"
+ runs-on: ubuntu-latest
+ steps:
+ - name: Free up disk space
+ run: |
+ # Preinstalled tools use a lot of disk space, free up some space
+ # https://github.com/actions/runner-images/issues/2840
+ sudo rm -rf "$AGENT_TOOLSDIRECTORY"
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 #
v7.0.1
+ with:
+ fetch-depth: 0
+ persist-credentials: false
+ - name: Get Date
+ id: get-date
+ run: |
+ echo "today=$(/bin/date -u '+%Y%m%d')" >> $GITHUB_OUTPUT
+ - name: Cache Conda
+ uses: actions/cache/restore@55cc8345863c7cc4c66a329aec7e433d2d1c52a9 #
v6.1.0
+ with:
+ path: ~/conda_pkgs_dir
+ key: conda-${{ runner.os }}-${{ steps.get-date.outputs.today }}-${{
env.CACHE_NUMBER }}-${{ hashFiles('ci/**') }}
+ - uses:
conda-incubator/setup-miniconda@8ee1f361103df19b6f8c8655fd3967a8ecb162d5 #
v4.0.1
+ with:
+ miniforge-version: latest
+ use-only-tar-bz2: false
+ use-mamba: true
+ - name: Install Dependencies
+ run: |
+ ./ci/scripts/remamba.sh install -c conda-forge \
+ --file ci/conda_env_cpp.txt \
+ --file ci/conda_env_python.txt
+ - name: Work around ASAN issue (GH-1617)
+ run: |
+ sudo sysctl vm.mmap_rnd_bits=28
+ - name: Build SQLite Driver
+ env:
+ BUILD_ALL: "OFF"
+ BUILD_DRIVER_MANAGER: "ON"
+ BUILD_DRIVER_SQLITE: "ON"
+ run: |
+ ./ci/scripts/cpp_build.sh "$(pwd)" "$(pwd)/build"
+ ./ci/scripts/python_build.sh "$(pwd)" "$(pwd)/build"
+ - uses: astral-sh/setup-uv@bec219d24cd3e171d82865faccec33120bb574f4 #
v10.1.0
+ - name: Run SQLite validation suite
+ env:
+ ADBC_USE_ASAN: "ON"
+ ADBC_USE_UBSAN: "ON"
+ BUILD_ALL: "0"
+ BUILD_DRIVER_SQLITE: "1"
+ run: |
+ export PYTHONPATH="$(pwd)/python/adbc_driver_manager"
+ uv --project=./c/driver/sqlite/validation/ run \
+ ci/scripts/validate.sh "$(pwd)"
+
postgresql-build:
name: "PostgreSQL Build"
runs-on: ubuntu-latest
diff --git a/c/driver/framework/connection.h b/c/driver/framework/connection.h
index 46259eb077..3cb183d7a3 100644
--- a/c/driver/framework/connection.h
+++ b/c/driver/framework/connection.h
@@ -165,8 +165,7 @@ class Connection : public ObjectBase {
}
return driver::Option();
}
- return status::NotImplemented(Derived::kErrorPrefix, " Unknown connection
option ",
- key);
+ return status::NotFound(Derived::kErrorPrefix, " Unknown connection option
", key);
}
/// \internal
diff --git a/c/driver/sqlite/CMakeLists.txt b/c/driver/sqlite/CMakeLists.txt
index 9147d5d6e9..44a848f083 100644
--- a/c/driver/sqlite/CMakeLists.txt
+++ b/c/driver/sqlite/CMakeLists.txt
@@ -52,6 +52,8 @@ if(NOT ADBC_SQLITE_WITH_LOAD_EXTENSION)
set(ADBC_SQLITE_COMPILE_DEFINES "-DADBC_SQLITE_WITH_NO_LOAD_EXTENSION")
endif()
+configure_file("config.h.in" "config.h" ESCAPE_QUOTES)
+
add_arrow_lib(adbc_driver_sqlite
SOURCES
sqlite.cc
@@ -78,8 +80,11 @@ foreach(LIB_TARGET ${ADBC_LIBRARIES})
target_compile_definitions(${LIB_TARGET} PRIVATE ADBC_EXPORTING
${ADBC_SQLITE_COMPILE_DEFINES})
target_include_directories(${LIB_TARGET} SYSTEM
- PRIVATE ${REPOSITORY_ROOT}/c/
${REPOSITORY_ROOT}/c/include/
- ${SQLite3_INCLUDE_DIRS}
${REPOSITORY_ROOT}/c/driver)
+ PRIVATE ${REPOSITORY_ROOT}/c/
+ ${REPOSITORY_ROOT}/c/include/
+ ${SQLite3_INCLUDE_DIRS}
+ ${REPOSITORY_ROOT}/c/driver
+ ${CMAKE_BINARY_DIR})
if(NOT ADBC_DEFINE_COMMON_ENTRYPOINTS)
target_compile_definitions(${LIB_TARGET} PRIVATE
${ADBC_TARGET_COMPILE_DEFINITIONS})
diff --git a/c/driver/sqlite/validation/queries/ingest/string.txtcase
b/c/driver/sqlite/config.h.in
similarity index 93%
copy from c/driver/sqlite/validation/queries/ingest/string.txtcase
copy to c/driver/sqlite/config.h.in
index 42b5549d8f..461fdd001b 100644
--- a/c/driver/sqlite/validation/queries/ingest/string.txtcase
+++ b/c/driver/sqlite/config.h.in
@@ -15,7 +15,4 @@
// specific language governing permissions and limitations
// under the License.
-// part: metadata
-
-[tags]
-sql-type-name = "TEXT"
+#define SQLITE_DRIVER_VERSION_STRING "@ADBC_VERSION@"
diff --git a/c/driver/sqlite/meson.build b/c/driver/sqlite/meson.build
index c34b024864..e2e615033a 100644
--- a/c/driver/sqlite/meson.build
+++ b/c/driver/sqlite/meson.build
@@ -17,6 +17,12 @@
sqlite3_dep = dependency('sqlite3')
+configure_file(
+ input: 'config.h.in',
+ output: 'config.h',
+ configuration: {'ADBC_VERSION': meson.project_version()},
+)
+
time_t_size = meson.get_compiler('c').sizeof(
'time_t',
prefix: '#include <sys/types.h>',
diff --git a/c/driver/sqlite/sqlite.cc b/c/driver/sqlite/sqlite.cc
index d6f20857f9..f628b3d6ed 100644
--- a/c/driver/sqlite/sqlite.cc
+++ b/c/driver/sqlite/sqlite.cc
@@ -31,6 +31,7 @@
#include "driver/framework/database.h"
#include "driver/framework/statement.h"
#include "driver/framework/status.h"
+#include "driver/sqlite/config.h"
#include "driver/sqlite/statement_reader.h"
#if SQLITE_VERSION_NUMBER < 3037000
@@ -343,7 +344,7 @@ struct SqliteGetObjectsHelper : public
driver::GetObjectsHelper {
// XXX: because we're saving the SqliteQuery, we also need to save the
string builder
columns_query.Reset();
columns_query.Append(
- R"(SELECT cid, name, type, 'notnull', dflt_value FROM
pragma_table_info(%Q, %Q) WHERE NAME LIKE ?)",
+ R"(SELECT cid, name, type, "notnull", dflt_value FROM
pragma_table_info(%Q, %Q) WHERE NAME LIKE ?)",
table.data(), catalog.data());
std::string_view query;
UNWRAP_RESULT(query, columns_query.GetString());
@@ -630,6 +631,7 @@ class SqliteConnection : public
driver::Connection<SqliteConnection> {
}
Result<std::optional<std::string>> GetCurrentCatalogImpl() { return "main"; }
+ Result<std::optional<std::string>> GetCurrentSchemaImpl() { return ""; }
Result<std::unique_ptr<driver::GetObjectsHelper>> GetObjectsImpl() {
return std::make_unique<SqliteGetObjectsHelper>(conn_);
@@ -702,11 +704,10 @@ class SqliteConnection : public
driver::Connection<SqliteConnection> {
result.emplace_back(code, "ADBC SQLite Driver");
break;
case ADBC_INFO_DRIVER_VERSION:
- // TODO(lidavidm): fill in driver version
- result.emplace_back(code, "(unknown)");
+ result.emplace_back(code, "v" SQLITE_DRIVER_VERSION_STRING);
break;
case ADBC_INFO_DRIVER_ARROW_VERSION:
- result.emplace_back(code, NANOARROW_VERSION);
+ result.emplace_back(code, "v" NANOARROW_VERSION);
break;
case ADBC_INFO_DRIVER_ADBC_VERSION:
result.emplace_back(code, static_cast<int64_t>(ADBC_VERSION_1_1_0));
@@ -891,7 +892,7 @@ class SqliteStatement : public
driver::Statement<SqliteStatement> {
return status::fmt::InvalidState("{} Cannot set both {} and {}",
kErrorPrefix,
ADBC_INGEST_OPTION_TARGET_CATALOG,
ADBC_INGEST_OPTION_TEMPORARY);
- } else if (state.target_schema) {
+ } else if (state.target_schema && !state.target_schema->empty()) {
return status::fmt::NotImplemented("{} {} not supported", kErrorPrefix,
ADBC_INGEST_OPTION_TARGET_DB_SCHEMA);
} else if (!state.target_table) {
@@ -983,6 +984,9 @@ class SqliteStatement : public
driver::Statement<SqliteStatement> {
default:
break;
}
+ if (!(view.schema->flags & ARROW_FLAG_NULLABLE)) {
+ create_query.Append(" NOT NULL");
+ }
}
create_query.Append(")");
@@ -1013,7 +1017,12 @@ class SqliteStatement : public
driver::Statement<SqliteStatement> {
}
switch (state.table_does_not_exist_) {
case Base::TableDoesNotExist::kCreate: {
- UNWRAP_STATUS(::adbc::sqlite::SqliteQuery::Execute(conn_, create));
+ auto status = ::adbc::sqlite::SqliteQuery::Execute(conn_, create);
+ if (status.message().find("already exists") != std::string::npos) {
+ return Status(ADBC_STATUS_ALREADY_EXISTS, status.message());
+ } else if (!status.ok()) {
+ return status;
+ }
break;
}
case Base::TableDoesNotExist::kFail:
@@ -1034,18 +1043,25 @@ class SqliteStatement : public
driver::Statement<SqliteStatement> {
int rc = sqlite3_prepare_v2(conn_, insert.data(),
static_cast<int>(insert.size()),
&stmt, /*pzTail=*/nullptr);
if (rc != SQLITE_OK) {
+ std::string err = sqlite3_errmsg(conn_);
std::ignore = sqlite3_finalize(stmt);
InternalAdbcSqliteBinderRelease(&binder_);
+
+ std::string message =
+ fmt::format("failed to prepare: {}\nquery was: {}", err, insert);
if (is_autocommit) {
auto status = ::adbc::sqlite::SqliteQuery::Execute(conn_,
"ROLLBACK");
if (!status.ok()) {
- return status::fmt::Internal(
- "failed to prepare: {}\nquery was: {}\nfailed to rollback: {}",
- sqlite3_errmsg(conn_), insert, status.message());
+ message += fmt::format("\nfailed to rollback: {}",
status.message());
}
}
- return status::fmt::Internal("failed to prepare: {}\nquery was: {}",
- sqlite3_errmsg(conn_), insert);
+
+ // XXX: we don't have a good way of detecting schema conflicts vs
other errors
+ if (err.find("has no column") != std::string::npos ||
+ err.find("no such column") != std::string::npos) {
+ return Status(ADBC_STATUS_ALREADY_EXISTS, std::move(message));
+ }
+ return Status(ADBC_STATUS_INTERNAL, std::move(message));
}
}
assert(stmt != nullptr);
diff --git a/c/driver/sqlite/sqlite_test.cc b/c/driver/sqlite/sqlite_test.cc
index 5af3a5ed32..22f7c8afe3 100644
--- a/c/driver/sqlite/sqlite_test.cc
+++ b/c/driver/sqlite/sqlite_test.cc
@@ -19,6 +19,7 @@
#include <filesystem> // NOLINT [build/c++17]
#include <limits>
#include <optional>
+#include <regex>
#include <string>
#include <string_view>
#include <utility>
@@ -146,7 +147,7 @@ class SqliteQuirks : public adbc_validation::DriverQuirks {
bool supports_bulk_ingest_temporary() const override { return true; }
bool supports_concurrent_statements() const override { return true; }
bool supports_metadata_current_catalog() const override { return true; }
- bool supports_metadata_current_db_schema() const override { return false; }
+ bool supports_metadata_current_db_schema() const override { return true; }
bool supports_get_option() const override { return true; }
std::optional<adbc_validation::SqlInfoValue> supports_get_sql_info(
uint32_t info_code) const override {
@@ -154,7 +155,7 @@ class SqliteQuirks : public adbc_validation::DriverQuirks {
case ADBC_INFO_DRIVER_NAME:
return "ADBC SQLite Driver";
case ADBC_INFO_DRIVER_VERSION:
- return "(unknown)";
+ return "v1.";
case ADBC_INFO_VENDOR_NAME:
return "SQLite";
case ADBC_INFO_VENDOR_VERSION:
@@ -298,7 +299,9 @@ TEST_F(SqliteConnectionTest, GetInfoMetadata) {
}
case ADBC_INFO_DRIVER_VERSION: {
ArrowStringView val = ArrowArrayViewGetStringUnsafe(str_child,
offset);
- EXPECT_EQ("(unknown)", std::string(val.data, val.size_bytes));
+ EXPECT_TRUE(
+ std::regex_match(std::string(val.data, val.size_bytes),
+
std::regex("v[0-9]+\\.[0-9]+\\.[0-9]+(-SNAPSHOT)?")));
break;
}
case ADBC_INFO_VENDOR_NAME: {
diff --git a/c/driver/sqlite/validation/queries/ingest/string.txtcase
b/c/driver/sqlite/validation/queries/ingest/string.txtcase
index 42b5549d8f..a8c26c3bf7 100644
--- a/c/driver/sqlite/validation/queries/ingest/string.txtcase
+++ b/c/driver/sqlite/validation/queries/ingest/string.txtcase
@@ -19,3 +19,13 @@
[tags]
sql-type-name = "TEXT"
+
+// part: input
+
+// Put non-NULL value first to help SQLite type inference
+{"idx": 0, "value": ""}
+{"idx": 1, "value": "hello world"}
+{"idx": 2, "value": "Special chars: !@#$%^&*()_+-=[]{}|;:,.<>?/\"\\"}
+{"idx": 3, "value": "Unicode: こんにちは, 世界!"}
+{"idx": 4, "value": "A very long string that tests the limits of string
handling in various systems. This string is intentionally verbose to ensure
that databases can properly handle longer text values without truncation or
other issues."}
+{"idx": 5, "value": null}
diff --git a/c/driver/sqlite/validation/tests/sqlite.py
b/c/driver/sqlite/validation/tests/sqlite.py
index 24b40aa1e6..06e356259d 100644
--- a/c/driver/sqlite/validation/tests/sqlite.py
+++ b/c/driver/sqlite/validation/tests/sqlite.py
@@ -34,7 +34,7 @@ class SQLiteQuirks(model.DriverQuirks):
get_objects=True,
get_objects_constraints_foreign=True,
get_objects_constraints_primary=True,
- get_objects_constraints_unique=True,
+ get_objects_constraints_unique=False,
statement_bind=True,
statement_bulk_ingest=True,
statement_bulk_ingest_catalog=True,
diff --git a/c/driver_manager/adbc_driver_manager_test.cc
b/c/driver_manager/adbc_driver_manager_test.cc
index caa0dddad3..7cef29ab49 100644
--- a/c/driver_manager/adbc_driver_manager_test.cc
+++ b/c/driver_manager/adbc_driver_manager_test.cc
@@ -439,7 +439,7 @@ class SqliteQuirks : public adbc_validation::DriverQuirks {
case ADBC_INFO_DRIVER_NAME:
return "ADBC SQLite Driver";
case ADBC_INFO_DRIVER_VERSION:
- return "(unknown)";
+ return "v1.";
case ADBC_INFO_VENDOR_NAME:
return "SQLite";
case ADBC_INFO_VENDOR_VERSION:
@@ -449,6 +449,7 @@ class SqliteQuirks : public adbc_validation::DriverQuirks {
}
}
bool supports_metadata_current_catalog() const override { return true; }
+ bool supports_metadata_current_db_schema() const override { return true; }
std::string catalog() const override { return "main"; }
};
diff --git a/r/adbcsqlite/bootstrap.R b/r/adbcsqlite/bootstrap.R
index 3ff391146c..22faa25668 100644
--- a/r/adbcsqlite/bootstrap.R
+++ b/r/adbcsqlite/bootstrap.R
@@ -19,4 +19,12 @@ BOOTSTRAP_SCRIPT <- file.path("..", "tools", "bootstrap-c.R")
if (file.exists(BOOTSTRAP_SCRIPT)) {
source(BOOTSTRAP_SCRIPT)
+
+ # The R package does not use CMake, so generate its driver version header
here.
+ version <- read.dcf("DESCRIPTION")[1, "Version"]
+ config <- readLines("src/c/driver/sqlite/config.h.in")
+ writeLines(
+ gsub("@ADBC_VERSION@", version, config, fixed = TRUE),
+ "src/c/driver/sqlite/config.h"
+ )
}
diff --git a/r/tools/bootstrap-c.R b/r/tools/bootstrap-c.R
index 87e5f6e7fd..e7547aa4ee 100644
--- a/r/tools/bootstrap-c.R
+++ b/r/tools/bootstrap-c.R
@@ -17,7 +17,7 @@
exclude <- Sys.getenv("ADBC_R_BOOTSTRAP_EXCLUDE", "")
-source_files <- list.files("../../c", "\\.(h|c|cc|hpp)$", recursive = TRUE)
+source_files <- list.files("../../c", "\\.(h|c|cc|hpp|in)?$", recursive = TRUE)
# Apply excludes that apply to all drivers
source_files <- source_files[!grepl("_test\\.cc", source_files)]
diff --git a/ruby/test/test-connection.rb b/ruby/test/test-connection.rb
index b40229734e..ff587cd14d 100644
--- a/ruby/test/test-connection.rb
+++ b/ruby/test/test-connection.rb
@@ -105,6 +105,7 @@ class ConnectionTest < Test::Unit::TestCase
info = @connection.info
[
:vendor_version,
+ :driver_version,
:driver_arrow_version,
].each do |version_name|
next unless info.key?(version_name)
@@ -114,7 +115,7 @@ class ConnectionTest < Test::Unit::TestCase
vendor_name: "SQLite",
vendor_version: "X.Y.Z",
driver_name: "ADBC SQLite Driver",
- driver_version: "(unknown)",
+ driver_version: "X.Y.Z",
driver_arrow_version: "X.Y.Z",
driver_adbc_version: ADBC::VERSION_1_1_0,
},
@@ -169,7 +170,7 @@ class ConnectionTest < Test::Unit::TestCase
end
def test_driver_version
- assert_equal("(unknown)", normalize_version(@connection.driver_version))
+ assert_equal("X.Y.Z", normalize_version(@connection.driver_version))
end
def test_driver_arrow_version
@@ -178,6 +179,6 @@ class ConnectionTest < Test::Unit::TestCase
private
def normalize_version(version)
- version&.gsub(/\A\d+\.\d+\.\d+(?:-SNAPSHOT)?\z/, "X.Y.Z")
+ version&.gsub(/\Av?\d+\.\d+\.\d+(?:-SNAPSHOT)?\z/, "X.Y.Z")
end
end
