To review#13
Open
RagnarOkk11 wants to merge 30 commits into
Open
Conversation
Weekly dev (initial test merge)
Weekly dev
csv reader and csv writer are better structured, clang-format changed…
step 2: simple queries
initial version of queries, started adding codec, all tests for reader and writer were written and passed
feat: API of execution added, perfomance of convertation increased hi…
…pported. Huge remake of aggregation function is done
… with every memory limit now), SELECT(*) supported
…logic is readable now
first version of ready engine!
first version of functional and ready to go engine!
first version of functional and ready to go engine!
fix: tests fix
…plementing multithreading
…hread execution officially finished.
Weekly dev
some unknown change
There was a problem hiding this comment.
Pull request overview
This PR is a large refactor that reorganizes the engine into clearer modules (column/, io/, execution/, types/, utils/) and introduces a new pipeline-based execution layer with optional multithreading, along with updated CSV/columnar IO and tests.
Changes:
- Replaces the old monolithic execution/operator headers with a new pipeline executor + expression framework (
execution/*). - Reworks CSV→columnar write/read into
io/*and introduces new column abstractions/builders (column/*,types/*). - Updates build/test wiring (CMake + scripts) and adds/ports Catch2 tests for IO and string vectors.
Reviewed changes
Copilot reviewed 80 out of 82 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/VectorOfStrings.h | Switches to #pragma once and updates includes for the string-vector utility. |
| src/utils/ThreadPool.h | Adds a thread pool abstraction with a single-thread fallback when MT is disabled. |
| src/utils/Macro.h | Introduces centralized column-type dispatch macros. |
| src/utils/Concurrency.h | Adds MT/No-MT wrappers for atomics/mutex/condvar usage. |
| src/utils/Assert.h | Refactors header guard to #pragma once and trims unrelated macro content. |
| src/types/TimeUnit.h | Adds TimeUnit parsing utilities for time truncation/extract features. |
| src/types/Timestamp.h | Adds timestamp parse/format/truncate helpers. |
| src/types/String.h | Adds a small string helper utility (length). |
| src/types/Date.h | Adds date parse/format/truncate helpers. |
| src/types/ColumnType.h | Adds column type enum + metadata structure used by IO/execution. |
| src/ScalarExpressions.h | Removes old scalar expression definitions (replaced by execution/expressions). |
| src/read_write/test/test_csv_parser.cpp | Removes old CSV parser tests (replaced by src/io/test/*). |
| src/read_write/CSVTokenizer.cpp | Removes old CSV tokenizer implementation (replaced by src/io/CsvTokenizer.*). |
| src/read_write/CSVToColumnar.h | Removes old CSV→columnar API (replaced by src/io/CsvToColumnar.*). |
| src/read_write/CSVToColumnar.cpp | Removes old CSV→columnar implementation (replaced by src/io/CsvToColumnar.cpp). |
| src/read_write/CSVReader.h | Removes old CSV reader interface (replaced by src/io/CsvReader.*). |
| src/read_write/CSVReader.cpp | Removes old CSV reader implementation. |
| src/read_write/ColumnarWriter.h | Removes old columnar writer interface (replaced by src/io/ColumnarWriter.*). |
| src/read_write/ColumnarWriter.cpp | Removes old columnar writer implementation. |
| src/read_write/ColumnarReader.h | Removes old columnar reader interface (replaced by src/io/ColumnarReader.*). |
| src/read_write/ColumnarReader.cpp | Removes old columnar reader implementation. |
| src/Query.h | Removes old query ID enum header. |
| src/OperatorsBase.h | Removes old operator model in favor of pipeline-based execution. |
| src/OperatorsBase.cpp | Removes old operator implementations. |
| src/io/test/TestReaderWriter.cpp | Adds integration tests for CSV→columnar writing and reading metadata/typed columns. |
| src/io/test/TestCsvParser.cpp | Adds tests for CSV tokenizer/reader edge cases (EOF, empty fields, trailing newline). |
| src/io/CsvTokenizer.h | Renames/updates tokenizer header and adjusts buffering strategy. |
| src/io/CsvTokenizer.cpp | Adds new tokenizer implementation under io/. |
| src/io/CsvToColumnar.h | Adds new CSV→columnar API under io/. |
| src/io/CsvToColumnar.cpp | Implements CSV→columnar conversion using schema/column builders. |
| src/io/CsvReader.h | Adds new CSV reader interface that parses header into names/types. |
| src/io/CsvReader.cpp | Implements new CSV reader using type-dispatch macros. |
| src/io/ColumnarWriter.h | Adds new columnar writer interface + explicit file-format documentation. |
| src/io/ColumnarWriter.cpp | Implements writer, including string serialization and metadata writing. |
| src/io/ColumnarReader.h | Adds metadata table + reader API using pread and ColumnFactory. |
| src/io/ColumnarReader.cpp | Implements metadata parsing + typed column materialization. |
| src/FilterFunctions.h | Removes old filter function framework (replaced by execution/expressions). |
| src/FilterExpressions.h | Removes old filter expression framework (replaced by execution/expressions). |
| src/ExecutionLogic.h | Removes old physical plan builder (replaced by execution/ExecutionLogic.*). |
| src/ExecutionAPI.h | Removes old DataFrame API (replaced by execution/ExecutionApi.h). |
| src/execution/PipelineExecutor.h | Adds pipeline build context and a result sink operator. |
| src/execution/Pipeline.h | Adds pipeline runtime (source/transforms/sink) with optional threading. |
| src/execution/OperatorsBase.h | Adds new operator interfaces for scan/filter/scalar/limit and sink-source breakers. |
| src/execution/expressions/ScalarFunctions.h | Adds scalar function implementations (extract minute, trunc, length, regexp replace, etc.). |
| src/execution/expressions/ScalarExpressions.h | Adds scalar expression layer that builds scalar functions and updates schema. |
| src/execution/expressions/FilterFunctions.h | Adds filter function implementations (comparisons, like, boolean combinators). |
| src/execution/expressions/FilterExpressions.h | Adds filter expression layer for building filter functions from schema. |
| src/execution/expressions/AggregationExpressions.h | Adds aggregation expression layer for global/grouped aggregation builders. |
| src/execution/expressions/AggExpHelper.h | Adds shared helpers for iterating/dispatching column data with selection vectors. |
| src/execution/ExecutionLogic.h | Adds logical plan nodes that build one or more pipelines. |
| src/execution/ExecutionLogic.cpp | Implements pipeline building for scan/filter/aggregate/order/limit/scalar/drop/reorder. |
| src/execution/ExecutionHelper.h | Adds formatting, key hashing, and copy helpers used in execution. |
| src/execution/ExecutionApi.h | Adds new user-facing DataFrame/DataResult API over the pipeline engine. |
| src/Engine.h | Removes legacy engine wrapper. |
| src/Engine.cpp | Removes legacy engine implementation. |
| src/column/TemporalColumn.h | Adds temporal column types + builders (DATE/TIMESTAMP). |
| src/column/StringColumn.h | Adds string column + builder. |
| src/column/Schema.h | Updates schema includes and removes old reader-based schema loading. |
| src/column/NumericColumn.h | Adds numeric columns + parsing traits + builders. |
| src/column/ColumnView.h | Adds flat/const column view abstractions used by CASE WHEN. |
| src/column/ColumnFactory.h | Adds factory for columns and column builders using macro dispatch. |
| src/column/ColumnBuilder.h | Adds base class for column builders + builder trait declaration. |
| src/column/Column.h | Adds new minimal Column interface. |
| src/column/Codec.h | Adds initial codec abstractions and example codecs (WIP). |
| src/column/CharColumn.h | Adds char column + builder. |
| src/CMakeLists.txt | Rewrites build targets/includes and adds MT target + new tests. |
| src/AggregationFunctions.h | Removes old aggregation framework (replaced by execution/expressions/*). |
| src/AggregationExpressions.h | Removes old aggregation expressions (replaced by execution/expressions/*). |
| script/setup.sh | Updates environment provisioning (Kitware repo + cmake install). |
| script/run_query.sh | Adds optional cold-cache run and safer output/log directory creation. |
| script/run_query_mt.sh | Adds MT runner script for columnar-engine-mt. |
| script/run_all_queries.sh | Adds batch runner to execute all queries with iterations/debug/answers/cold-cache. |
| script/run_all_queries_mt.sh | Adds MT batch runner counterpart. |
| script/build.sh | Builds all targets (not just a single executable target). |
| CMakeLists.txt | Adds FetchContent for abseil + re2 and updates minimum required CMake version. |
| .gitignore | Updates ignored artifacts (logs, larger datasets). |
| src/utils/test/TestVectorOfStrings.cpp | Expands/cleans VectorOfStrings tests and includes. |
Comments suppressed due to low confidence (1)
src/io/CsvTokenizer.h:21
- kBufferSize is set to 1<<27 (~134 MiB), which forces every CsvTokenizer instance to allocate a very large buffer up front. This can cause high memory usage / OOM in tests or when multiple tokenizers exist. Consider using a smaller default (e.g. 1<<20 like before) or making it configurable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+19
to
+23
| #else | ||
|
|
||
| #include "utils/Assert.h" | ||
|
|
||
| template <typename T> |
Comment on lines
+7
to
+11
| #include <array> | ||
| #include <fcntl.h> | ||
| #include <fstream> | ||
| #include <unistd.h> | ||
|
|
Comment on lines
+109
to
+118
| void ColumnarReader::ReadRawColumnData(size_t column_index, size_t chunk_index, | ||
| std::vector<char>& buffer) const { | ||
| const ColumnMetadata& meta = (*metadata_)[column_index]; | ||
|
|
||
| if (column_index >= metadata_->GetNumColumns()) { | ||
| THROW_RUNTIME_ERROR("Column index out of range"); | ||
| } | ||
| if (chunk_index >= meta.offsets.size()) { | ||
| THROW_RUNTIME_ERROR("Chunk index out of range"); | ||
| } |
Comment on lines
+1
to
+5
| #pragma once | ||
|
|
||
| #include "execution/OperatorsBase.h" | ||
| #include "execution/expressions/AggExpHelper.h" | ||
|
|
Comment on lines
+55
to
+59
| if (fields[i].name == column_name) { | ||
| if (!batches_[0]->columns.empty()) { | ||
| return batches_[0]->columns[i]; | ||
| } | ||
| break; |
Comment on lines
+53
to
+55
| Pipeline* outer_pipeline = ctx.current_pipeline; | ||
| Pipeline* inner_pipeline = new Pipeline(); // TODO: this is bad, should not use raw pointers | ||
| ctx.current_pipeline = inner_pipeline; |
Comment on lines
+153
to
+155
| Pipeline* outer_pipeline = ctx.current_pipeline; | ||
| Pipeline* inner_pipeline = new Pipeline(); // TODO: this is bad, should not use raw pointers | ||
| ctx.current_pipeline = inner_pipeline; |
Comment on lines
+35
to
+39
| uint64_t metadata_start; | ||
| file.read(reinterpret_cast<char*>(&metadata_start), sizeof(metadata_start)); | ||
|
|
||
| size_t metadata_size = (file_size - 20) - metadata_start; | ||
| std::vector<char> buffer(metadata_size); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.