Skip to content

Conversation

@weidqi
Copy link

@weidqi weidqi commented Aug 10, 2025

Add support for the DuckDB

Copy link

Copilot AI left a 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_statements API
  • 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;
Copy link

Copilot AI Nov 25, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines 394 to 405
thisPtr->batchSqlCommands_ = std::move(sqlCommands);
thisPtr->executeBatchSql();
});
}

/**
* @brief 执行批量 SQL 的主逻辑
*/
void DuckdbConnection::executeBatchSql()
{
loop_->assertInLoopThread();

Copy link

Copilot AI Nov 25, 2025

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.

Suggested change
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);

Copilot uses AI. Check for mistakes.
@hwc0919
Copy link
Member

hwc0919 commented Nov 25, 2025

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.

Copy link
Member

@marty1885 marty1885 left a 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..

  1. Why is utf8proc added? I don't see it being used and complicating our license

.gitignore Outdated
*.out
*.app

trantor/
Copy link
Member

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?

Copy link
Author

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 直接实现
Copy link
Member

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

Comment on lines +476 to +492
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()
Copy link
Member

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 + ");";
Copy link
Member

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;
Copy link
Member

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh,thanks

@weidqi
Copy link
Author

weidqi commented Nov 26, 2025

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.

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.

weidqi and others added 7 commits November 27, 2025 08:55
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>
@weidqi weidqi requested a review from marty1885 November 28, 2025 08:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants