Skip to content

perf: DirectIndexAgg for low-cardinality integer GROUP BY #157

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.

Sign up for GitHub

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

Open
poyrazK wants to merge 8 commits into
base: main
Choose a base branch
from
Open

perf: DirectIndexAgg for low-cardinality integer GROUP BY #157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ce1f621
Performance investigation: GROUP BY aggregation fixes
poyrazK May 18, 2026
d295dc0
Re-implement parallel scan with ThreadPool and steal() method
poyrazK May 19, 2026
d034935
Add DirectIndexAgg for low-cardinality integer GROUP BY
poyrazK May 21, 2026
d86ab76
Fix vectorized path activation for GROUP BY with unanalyzed tables
poyrazK May 21, 2026
864bbca
style: automated clang-format fixes
poyrazK May 21, 2026
d63b763
Clean up DirectIndexAgg: remove dead code, mark slot valid in find_or…
poyrazK May 21, 2026
0f02154
Add OpenAddressHashAgg class for efficient hash-based aggregation
poyrazK May 21, 2026
adff7ce
style: automated clang-format fixes
poyrazK May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benchmarks/duckdb_comparison_bench.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ struct CloudSQLContext {
txn_manager = std::make_unique<transaction::TransactionManager>(*lock_manager, *catalog, *bpm);
executor = std::make_unique<QueryExecutor>(*catalog, *bpm, *lock_manager, *txn_manager);
executor->set_local_only(true);
executor->set_storage_manager(storage.get()); // Enable use_vectorized for large scans

// Create lineitem table (TPC-H schema, simplified)
CreateTableStatement create_stmt;
Expand Down
23 changes: 23 additions & 0 deletions include/executor/types.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,13 @@ class ColumnVector {
size_ = 0;
null_bitmap_.clear();
}

/**
* @brief Steals data from another column vector by swapping internal buffers.
* After steal(), 'other' is emptied and 'this' holds the original data from 'other'.
* Throws std::runtime_error if types are incompatible.
*/
virtual void steal(ColumnVector&& other) = 0;
Comment on lines +247 to +252
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Semantic contract mismatch between documentation and implementation.

The documentation states "After steal(), 'other' is emptied," but the implementations (lines 339-345, 411-417) only swap internal buffers without clearing other. After steal(), both vectors remain valid with swapped contents rather than other being empty. This violates the documented contract and could mislead callers who rely on other being emptied.

🔄 Proposed fix: clear 'other' after swapping

Update implementations to explicitly clear other after swapping:

 void steal(ColumnVector&& other) override {
     auto* other_num = dynamic_cast<NumericVector<T>*>(&other);
     if (!other_num) throw std::runtime_error("NumericVector::steal: type mismatch");
     data_.swap(other_num->data_);
     null_bitmap_.swap(other_num->null_bitmap_);
     std::swap(size_, other_num->size_);
+    other_num->data_.clear();
+    other_num->null_bitmap_.clear();
+    other_num->size_ = 0;
 }

Apply the same pattern to StringVector::steal at lines 411-417.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@include/executor/types.hpp` around lines 247 - 252, The doc for virtual void
steal(ColumnVector&& other) promises that 'other' is emptied after stealing but
current implementations only swap buffers; update each steal implementation
(e.g., ColumnVector::steal and StringVector::steal) to perform the swap and then
explicitly clear or reset the moved-from 'other' (call its clear/reset method,
set size/length to zero, and release/empty any auxiliary buffers) so that after
the call 'other' is empty while 'this' holds the original data; ensure you use
the class-specific clear/reset APIs to leave 'other' in a valid empty state.

};

/**
Expand Down Expand Up @@ -328,6 +335,14 @@ class NumericVector : public ColumnVector {
ColumnVector::clear();
data_.clear();
}

void steal(ColumnVector&& other) override {
auto* other_num = dynamic_cast<NumericVector<T>*>(&other);
if (!other_num) throw std::runtime_error("NumericVector::steal: type mismatch");
data_.swap(other_num->data_);
null_bitmap_.swap(other_num->null_bitmap_);
std::swap(size_, other_num->size_);
}
};

/**
Expand Down Expand Up @@ -392,6 +407,14 @@ class StringVector : public ColumnVector {
ColumnVector::clear();
data_.clear();
}

void steal(ColumnVector&& other) override {
auto* other_str = dynamic_cast<StringVector*>(&other);
if (!other_str) throw std::runtime_error("StringVector::steal: type mismatch");
data_.swap(other_str->data_);
null_bitmap_.swap(other_str->null_bitmap_);
std::swap(size_, other_str->size_);
}
};

/**
Expand Down
Loading