Upgrade DuckDB and extension CI tools to version 1.5.0#261
Conversation
… icon to documentation. (#253)
There was a problem hiding this comment.
Pull request overview
Updates the Flock DuckDB extension to target DuckDB/extension-ci-tools v1.5.0, adjusting build/test wiring and extension hook registration to match the newer DuckDB APIs.
Changes:
- Bump CI workflows and docs to DuckDB v1.5.0.
- Update extension hook registration (parser/operator) to use registration APIs rather than direct config vector mutation.
- Adjust unit-test linking and extension build configuration (dependency loading + coverage build type support).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/CMakeLists.txt | Refines test source discovery and expands link set for DuckDB 1.5.0-era targets. |
| src/include/flock_extension.hpp | Adds DuckDB extension hook headers needed for new registration APIs. |
| src/functions/scalar/llm_filter/implementation.cpp | Fixes llm_filter to broadcast single-result outputs across the chunk. |
| src/flock_extension.cpp | Switches parser/operator hook setup to ParserExtension::Register / OperatorExtension::Register. |
| src/core/config/model.cpp | Removes explicit INSTALL/LOAD JSON during config table setup. |
| README.md | Updates stated DuckDB version requirement to 1.5.0+. |
| extension_config.cmake | Loads core_functions and json before loading flock in DuckDB’s build system. |
| CMakeLists.txt | Adds a Coverage build type and reorganizes coverage flag setup timing. |
| .github/workflows/MainDistributionPipeline.yml | Pins extension CI workflows and DuckDB version to v1.5.0. |
| .github/copilot-instructions.md | Updates documented targeted DuckDB version to v1.5.0. |
Comments suppressed due to low confidence (2)
src/core/config/model.cpp:18
model_argsis declared asJSON, but this function no longer ensures the JSON extension is available before creating the table. If JSON isn’t built-in/auto-loaded in DuckDB 1.5.0 for all environments, extension initialization may fail here. Please ensure the JSON extension is loaded as a runtime dependency (not just viaextension_config.cmake), or avoid theJSONtype in these config tables.
void Config::SetupDefaultModelsConfig(duckdb::Connection& con, std::string& schema_name) {
const std::string table_name = Config::get_default_models_table_name();
con.Query(duckdb_fmt::format(" CREATE TABLE IF NOT EXISTS {}.{} ( "
" model_name VARCHAR NOT NULL PRIMARY KEY, "
" model VARCHAR NOT NULL, "
" provider_name VARCHAR NOT NULL, "
" model_args JSON DEFAULT '{{}}'"
" ); ",
schema_name, table_name));
src/core/config/model.cpp:41
- Same concern as above: this table uses a
JSONcolumn type but no longer installs/loads the JSON extension beforehand. If JSON isn’t guaranteed to be available, this can break first-run configuration. Please ensure JSON is loaded as a runtime dependency before executing this DDL.
void Config::SetupUserDefinedModelsConfig(duckdb::Connection& con, std::string& schema_name) {
const std::string table_name = Config::get_user_defined_models_table_name();
con.Query(duckdb_fmt::format(" CREATE TABLE IF NOT EXISTS {}.{} ( "
" model_name VARCHAR NOT NULL PRIMARY KEY, "
" model VARCHAR NOT NULL, "
" provider_name VARCHAR NOT NULL, "
" model_args JSON NOT NULL"
" ); ",
schema_name, table_name));
| file(GLOB_RECURSE TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/*.cpp) | ||
| list(FILTER TEST_SOURCES EXCLUDE REGEX ".*test_main\\.cpp$") |
There was a problem hiding this comment.
list(FILTER ...) requires CMake >= 3.6, but the repo’s cmake_minimum_required is 3.5. This will break configuration with the documented minimum CMake version. Either bump the minimum required CMake version, or replace this with a 3.5-compatible removal (e.g., list(REMOVE_ITEM TEST_SOURCES "${CMAKE_CURRENT_SOURCE_DIR}/test_main.cpp")).
| # Coverage instrumentation (must be before targets are built) | ||
| if(CMAKE_BUILD_TYPE STREQUAL "Coverage") | ||
| message(STATUS "Enabling code coverage") | ||
| add_compile_options(-fprofile-instr-generate -fcoverage-mapping) | ||
| add_link_options(-fprofile-instr-generate -fcoverage-mapping) | ||
| endif() |
There was a problem hiding this comment.
The Coverage build type unconditionally adds Clang coverage flags (-fprofile-instr-generate -fcoverage-mapping). These flags will fail on GCC toolchains. Consider guarding by CMAKE_CXX_COMPILER_ID MATCHES "Clang" (and using --coverage/-fprofile-arcs -ftest-coverage for GNU), or documenting that Coverage builds require Clang.
| if (const auto results = LlmFilter::Operation(args, bind_data); static_cast<int>(results.size()) == 1) { | ||
| auto empty_vec = duckdb::Vector(std::string()); | ||
| duckdb::UnaryExecutor::Execute<duckdb::string_t, duckdb::string_t>( | ||
| empty_vec, result, args.size(), | ||
| [&](duckdb::string_t name) { return duckdb::StringVector::AddString(result, results[0]); }); |
There was a problem hiding this comment.
The results.size() == 1 branch broadcasts by executing a per-row lambda that calls StringVector::AddString for every row, which duplicates the same string in the result heap. Prefer using DuckDB’s constant vector facilities to set a single constant value for the whole chunk to reduce allocations and memory use.
No description provided.