-
Notifications
You must be signed in to change notification settings - Fork 17
Integrations tests for CH SQL Generation #20
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
Conversation
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
Adds infrastructure and tests to validate ClickHouse SQL generation, refactors messaging to support rich content parts, and enhances multi-step coordination and request builders for tool calls and results.
- Integrates
clickhouse-cppas a submodule with CMake support and adds ClickHouse integration tests. - Refactors
Messagemodel to a variant-based content-part approach and updates test and parser code accordingly. - Extends
MultiStepCoordinator, OpenAI/Anthropic request builders, and response parsers to handle tool calls and tool results.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/clickhouse-cpp | Added submodule commit for the ClickHouse C++ client |
| third_party/clickhouse-cmake/CMakeLists.txt | Wrapper to expose ClickHouse client as an interface target |
| third_party/CMakeLists.txt | Includes clickhouse-cmake when BUILD_TESTS is enabled |
| tests/utils/test_fixtures.cpp | Updated Message creation to use new factory methods |
| tests/unit/types_test.cpp | Updated tests to use Message::user/system/assistant and get_text |
| tests/integration/openai_integration_test.cpp | Removed unused includes and updated Message usage |
| tests/integration/anthropic_integration_test.cpp | Same updates for Anthropic integration tests |
| tests/integration/clickhouse_integration_test.cpp | New end-to-end tests for ClickHouse SQL generation |
| tests/CMakeLists.txt | Registers the ClickHouse integration test in the test suite |
| test-services/clickhouse/docker-compose.yaml | Defines a ClickHouse service for integration testing |
| src/tools/multi_step_coordinator.cpp | Adjusted function signature, added logging, and tool-call support |
| include/ai/tools.h | Aligned function signature for execute_multi_step |
| src/providers/openai/openai_response_parser.cpp | Uses new Message::assistant factory for responses |
| src/providers/anthropic/anthropic_response_parser.cpp | Same update for Anthropic responses |
| src/providers/openai/openai_request_builder.cpp | Serializes text, tool calls, and tool results in JSON requests |
| src/providers/anthropic/anthropic_request_builder.cpp | Same serialization support for Anthropic |
| include/ai/types/message.h | New variant-based MessageContent model with content-part factories |
| include/ai/types/tool.h | Cleaned up forward declarations and introduced ToolExecutionContext |
| include/ai/openai.h | Added new model constants for O-series and GPT-4.1 models |
| .gitmodules | Registered the clickhouse-cpp submodule |
Comments suppressed due to low confidence (1)
src/providers/openai/openai_request_builder.cpp:21
- Add unit tests to verify that
OpenAIRequestBuildercorrectly serializes messages with tool results into JSON, covering both successful and error result cases.
if (msg.has_tool_results()) {
|
|
||
| // Build the messages for the next step | ||
| Messages next_messages = next_options.messages; | ||
|
|
Copilot
AI
Jul 14, 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.
The system prompt (base_options.system) is no longer propagated into next_messages for subsequent steps, which may lead to loss of context in multi-step generations. Consider re-inserting the system message before the user prompt when base_options.system is non-empty.
| // Add system prompt if non-empty | |
| if (!base_options.system.empty()) { | |
| next_messages.insert(next_messages.begin(), Message::system(base_options.system)); | |
| } |
| static std::random_device rd; | ||
| static std::mt19937 gen(rd()); | ||
| static std::uniform_int_distribution<> dis(0, sizeof(alphabet) - 2); |
Copilot
AI
Jul 14, 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 expression sizeof(alphabet) - 2 is a magic number; consider defining a named constant (e.g., alphabet_length) to clarify the valid index range and improve readability.
| static std::random_device rd; | |
| static std::mt19937 gen(rd()); | |
| static std::uniform_int_distribution<> dis(0, sizeof(alphabet) - 2); | |
| static constexpr size_t alphabet_length = sizeof(alphabet) - 1; // Exclude null terminator | |
| static std::random_device rd; | |
| static std::mt19937 gen(rd()); | |
| static std::uniform_int_distribution<> dis(0, alphabet_length - 1); |
No description provided.