Conversation
There was a problem hiding this comment.
Pull request overview
Adds an initial end-to-end “run” path for pulling tick data from QuestDB, parsing a Base64 strategy config, and iterating through streamed ticks (with supporting model/API updates).
Changes:
- Introduces Base64→JSON configuration parsing and a new
Operations::run()tick-processing entry point. - Reworks DB access to build symbol-union queries and parse results into an updated
PriceDatamodel that includesask/bid/timestamp/symbol. - Updates CMake/OpenMP linkage and scripts/docs to support running the new flow.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| source/utilities/jsonParser.cpp | New Base64 JSON configuration parser used by CLI. |
| source/sqlManager.cpp | Builds multi-symbol query and calls DB streaming API. |
| source/operations.cpp | Adds tick loop/printing “operations” runner. |
| source/main.cpp | Wires config parsing + tick streaming + operations run into CLI. |
| source/databaseConnection.cpp | Adds faster timestamp parsing and new streamQuery() result parsing. |
| source/CMakeLists.txt | Adds a duplicate CMake build file for the source/ subtree. |
| scripts/run.sh | Adds build-failure guard and execution timing. |
| scripts/build_dep.sh | Adds a helper script to build libpqxx dependency. |
| include/sqlManager.hpp | Updates SqlManager API to streamPriceData/symbol list. |
| include/operations.hpp | Declares new Operations runner. |
| include/models/priceData.hpp | Updates tick model fields and constructors. |
| include/databaseConnection.hpp | Adds streamQuery() declaration (keeps executeQuery() declaration). |
| documents/questdb.md | Adds a short note on starting QuestDB (macOS). |
| CMakeLists.txt | Adds OpenMP linking for the main build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| std::string query; | ||
| for (size_t i = 0; i < symbols.size(); ++i) { | ||
| if (i > 0) { | ||
| query += " UNION ALL "; | ||
| } | ||
| query += "SELECT '" + symbols[i] + "' as symbol, * FROM '" + symbols[i] + "' WHERE timestamp >= dateadd('M', -" + std::to_string(LAST_MONTHS) + ", now())"; | ||
| } |
There was a problem hiding this comment.
The generated SQL uses single quotes around the table name (FROM 'EURUSD'), which in SQL denotes a string literal, not an identifier. This will produce invalid SQL (or query the wrong thing). Use an unquoted identifier or proper identifier quoting (typically double quotes), and ensure symbols are validated/escaped to avoid SQL injection via symbols[i].
| std::string SqlManager::getBaseQuery(const std::vector<std::string>& symbols, int LAST_MONTHS) { | ||
| if (symbols.empty()) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
getBaseQuery returns an empty string when symbols is empty, but streamPriceData will still execute the query string it gets back. Returning an empty query can lead to runtime SQL errors; consider returning an empty result early in streamPriceData (or throwing) when symbols is empty.
| struct tm tm = {}; | ||
| if (localtime_r(&time_t, &tm) == nullptr) { | ||
| std::cerr << "Error: failed to convert timestamp" << std::endl; | ||
| } | ||
| char buffer[20]; | ||
| std::strftime(buffer, sizeof(buffer), "%Y-%m-%d %H:%M:%S", &tm); | ||
| auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(tick.timestamp.time_since_epoch()) % 1000; |
There was a problem hiding this comment.
If localtime_r fails, the code logs an error but then continues to call std::strftime using an uninitialized tm value. This can print garbage timestamps. Consider continue; (or otherwise handling the error) after the failure so the formatting code is skipped.
| if(APPLE) | ||
| set(OpenMP_C_FLAGS "-Xclang -fopenmp") | ||
| set(OpenMP_CXX_FLAGS "-Xclang -fopenmp") | ||
| set(OpenMP_C_LIB_NAMES "omp") | ||
| set(OpenMP_CXX_LIB_NAMES "omp") | ||
| set(OpenMP_omp_LIBRARY /opt/homebrew/opt/libomp/lib/libomp.dylib) | ||
| find_package(OpenMP REQUIRED) | ||
| target_include_directories(BacktestingEngineLib PRIVATE /opt/homebrew/opt/libomp/include) | ||
| endif() | ||
|
|
||
| target_link_libraries(BacktestingEngineLib PUBLIC pqxx OpenMP::OpenMP_CXX) | ||
|
|
There was a problem hiding this comment.
This CMake file links OpenMP::OpenMP_CXX unconditionally, but find_package(OpenMP REQUIRED) is only called inside if(APPLE). On non-Apple platforms this will fail because the imported target won’t exist. Consider calling find_package(OpenMP REQUIRED) for all platforms (keeping only the Apple-specific hints in the if(APPLE) block) or conditionally linking OpenMP when found.
| cd ./external/libpqxx | ||
|
|
||
| mkdir -p build |
There was a problem hiding this comment.
This script is missing a shebang (e.g., #!/bin/bash or #!/usr/bin/env bash). As-is, running it directly (instead of sh scripts/build_dep.sh) may fail depending on permissions/default shell. Add a shebang and consider set -euo pipefail so dependency builds fail fast.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…com:mccaffers/backtesting-engine-cpp into 10-create-end-to-end-trade-management-logic
… in it's own child process
|
No description provided.