-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Duckdb add branch_alpha #2369
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Duckdb add branch_alpha #2369
Conversation
Removed CLAUDE.md from .gitignore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds comprehensive DuckDB database support to the Drogon web framework. DuckDB is an embedded analytical database similar to SQLite but optimized for analytical workloads. The implementation follows the existing patterns used for PostgreSQL, MySQL, and SQLite3 support, providing connection management, result handling, parameter binding, batch operations, and flexible configuration options.
Key Changes:
- Implementation of DuckDB connection and result classes using the DuckDB C API
- Flexible configuration system supporting DuckDB-specific options (threads, memory limits, access mode)
- Batch SQL execution support using
duckdb_extract_statementsAPI - Integration with existing Drogon ORM infrastructure and code generation tools
Reviewed changes
Copilot reviewed 34 out of 36 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
orm_lib/src/duckdb_impl/DuckdbConnection.cc |
Core connection implementation with SQL execution, parameter binding, and batch operations |
orm_lib/src/duckdb_impl/DuckdbResultImpl.cc |
Result set implementation with type conversion and value caching |
orm_lib/inc/drogon/orm/DbConfig.h |
Added DuckdbConfig structure with configOptions support |
orm_lib/src/DbClient.cc |
Added factory methods for creating DuckDB clients |
orm_lib/src/DbClientImpl.cc |
Client implementation with configuration option handling |
lib/src/ConfigLoader.cc |
JSON config parsing for DuckDB config_options |
CMakeLists.txt |
Build system updates for DuckDB detection and compilation |
orm_lib/src/duckdb_impl/test/duck_test.cc |
Comprehensive test suite with 20+ test categories |
third_party/utf8proc/* |
UTF8 processing library files (appears to be third-party code) |
Comments suppressed due to low confidence (1)
orm_lib/src/DbClientImpl.cc:1
- Code and its comment were removed without clear justification. The comment indicates this handles cleanup of invalid connection pointers. If this is intentional cleanup, ensure the disconnect logic is handled elsewhere or the removal is documented.
/**
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #include "duckdb/common/exception.hpp" | ||
| #include "duckdb/common/helper.hpp" | ||
|
|
||
| using namespace std; |
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'using namespace std;' in a header or source file that may be included elsewhere is generally discouraged as it pollutes the global namespace. Consider using explicit std:: prefixes instead.
| thisPtr->batchSqlCommands_ = std::move(sqlCommands); | ||
| thisPtr->executeBatchSql(); | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * @brief 执行批量 SQL 的主逻辑 | ||
| */ | ||
| void DuckdbConnection::executeBatchSql() | ||
| { | ||
| loop_->assertInLoopThread(); | ||
|
|
Copilot
AI
Nov 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The sqlCommands are moved into the lambda, then moved again into batchSqlCommands_. This is correct but creates an extra move operation. Consider moving directly: thisPtr->executeBatchSql(std::move(sqlCommands)); and updating executeBatchSql to accept parameters.
| thisPtr->batchSqlCommands_ = std::move(sqlCommands); | |
| thisPtr->executeBatchSql(); | |
| }); | |
| } | |
| /** | |
| * @brief 执行批量 SQL 的主逻辑 | |
| */ | |
| void DuckdbConnection::executeBatchSql() | |
| { | |
| loop_->assertInLoopThread(); | |
| thisPtr->executeBatchSql(std::move(sqlCommands)); | |
| }); | |
| } | |
| /** | |
| * @brief 执行批量 SQL 的主逻辑 | |
| */ | |
| void DuckdbConnection::executeBatchSql(std::deque<std::shared_ptr<SqlCmd>> &&sqlCommands) | |
| { | |
| loop_->assertInLoopThread(); | |
| batchSqlCommands_ = std::move(sqlCommands); |
|
It's really bad to integrate various niche products into drogon source code. I don't encourage it, nor do I like such attemps. We should enhance our plugin api, and let user write 3rd-party plugins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow this is a crap ton of code..
- Why is utf8proc added? I don't see it being used and complicating our license
.gitignore
Outdated
| *.out | ||
| *.app | ||
|
|
||
| trantor/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a submodule. Why is it in the ignore list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me fix it
CMakeLists.txt
Outdated
| target_link_libraries(${PROJECT_NAME} PRIVATE ${DuckDB_LIBRARIES}) | ||
| target_include_directories(${PROJECT_NAME} PRIVATE ${DuckDB_INCLUDE_DIRS}) | ||
| set(DROGON_FOUND_DUCKDB TRUE) | ||
| # 使用 duckdb_impl 目录下的 DuckDB C API 直接实现 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For global development. I prefer keeping comments English
| if(NOT DuckDB_FOUND) | ||
| # Try to find DuckDB manually | ||
| find_library(DUCKDB_LIBRARY | ||
| NAMES duckdb | ||
| PATHS /usr/lib /usr/local/lib /opt/homebrew/lib | ||
| ) | ||
| find_path(DUCKDB_INCLUDE_DIR | ||
| NAMES duckdb.h | ||
| PATHS /usr/include /usr/local/include /opt/homebrew/include | ||
| ) | ||
|
|
||
| if(DUCKDB_LIBRARY AND DUCKDB_INCLUDE_DIR) | ||
| set(DuckDB_LIBRARIES ${DUCKDB_LIBRARY}) | ||
| set(DuckDB_INCLUDE_DIRS ${DUCKDB_INCLUDE_DIR}) | ||
| set(DuckDB_FOUND TRUE) | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled in FindDuckDB.
| std::vector<ColumnInfo> cols; | ||
|
|
||
| // 使用 PRAGMA table_info 查询表结构(DuckDB 兼容 SQLite) | ||
| std::string sql = "PRAGMA table_info(" + tableName + ");"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels possible to SQLi here?
| [&](bool isNull, std::string &&tableName) mutable { | ||
| if (!isNull) | ||
| { | ||
| std::cout << "table name:" << tableName << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use trantor's LOG_* instead of cout or cerr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh,thanks
I would certainly like to use a plugin approach for extension, but I haven't found a good implementation path for extending the ORM using plugins. If you know of one, please let me know and help us improve Drogon's support for more databases. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…TARGET_FILE:_drogon_ctl>
Removed directories 'trantor/'
Add support for the DuckDB