Release: OpenAddressHashAgg optimizations + PostgreSQL benchmark#164
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 5 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds a PostgreSQL comparison benchmark suite to measure cloudSQL's vectorized SQL engine against PostgreSQL, and optimizes GROUP BY aggregation through sentinel-based MIN/MAX tracking and batch accumulation. The benchmark covers OLTP (INSERT, UPDATE, SELECT) and analytical (SCAN, GROUP BY, JOIN, filtered queries) patterns, while aggregation improvements eliminate conditional branching and support multi-row batch updates. ChangesPostgreSQL Comparison Benchmark Suite
GROUP BY Aggregation Optimization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
include/executor/vectorized_operator.hpp (2)
1170-1234:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLogic error in
update_aggregate_batch: MIN/MAX branch is unreachable for int64/float64 columns.The conditional structure is:
if (col.type() == TYPE_INT64) { /* sums */ } else if (col.type() == TYPE_FLOAT64) { /* sums */ } else if (agg.type == Min || agg.type == Max) { /* min/max */ }The MIN/MAX branch (line 1207) only executes when
col.type()is neitherTYPE_INT64norTYPE_FLOAT64. For numeric MIN/MAX aggregates on int64 or float64 columns, the sum-update branches execute instead, producing incorrect results.Since this helper is not yet called (scaffolding for future batch processing per PR summary), this is not a runtime bug today, but will cause incorrect aggregation when integrated.
Suggested fix
// Type resolved ONCE, then batch process all rows - if (col.type() == common::ValueType::TYPE_INT64) { + if (agg.type == AggregateType::Min || agg.type == AggregateType::Max) { + // MIN/MAX with sentinel-based approach + if (col.type() == common::ValueType::TYPE_INT64) { + const auto& num_col = static_cast<const NumericVector<int64_t>&>(col); + const int64_t* raw = num_col.raw_data(); + for (size_t j = 0; j < num_rows; ++j) { + size_t r = row_indices[j]; + if (!num_col.is_null(r)) { + bucket.mins[agg_idx] = std::min(bucket.mins[agg_idx], raw[r]); + bucket.maxes[agg_idx] = std::max(bucket.maxes[agg_idx], raw[r]); + bucket.has_mins[agg_idx] = true; + } + } + } else if (col.type() == common::ValueType::TYPE_FLOAT64) { + const auto& num_col = static_cast<const NumericVector<double>&>(col); + const double* raw = num_col.raw_data(); + for (size_t j = 0; j < num_rows; ++j) { + size_t r = row_indices[j]; + if (!num_col.is_null(r)) { + bucket.mins_float64[agg_idx] = std::min(bucket.mins_float64[agg_idx], raw[r]); + bucket.maxes_float64[agg_idx] = std::max(bucket.maxes_float64[agg_idx], raw[r]); + bucket.has_float_minmax[agg_idx] = true; + } + } + } + } else if (col.type() == common::ValueType::TYPE_INT64) { const auto& num_col = static_cast<const NumericVector<int64_t>&>(col); // ... existing sum logic ...🤖 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/vectorized_operator.hpp` around lines 1170 - 1234, The batch update logic in update_aggregate_batch incorrectly places the AggregateType::Min/Max handling after the TYPE_INT64/TYPE_FLOAT64 branches so numeric MIN/MAX never run; change the control flow so that agg.type is checked first for MIN/MAX (using aggregates_[agg_idx].type / agg.type) and then, inside that branch, dispatch on col.type() to update bucket.mins/mins_float64 and bucket.maxes/maxes_float64 (using NumericVector<int64_t>/NumericVector<double> and raw_data()), or alternatively keep the numeric-type branches but branch on agg.type inside them to perform sum (bucket.sums_int64/sums_float64 and bucket.has_float_value) for non-min/max and min/max updates for AggregateType::Min/Max; ensure Count and input_col_idx handling remains unchanged.
1150-1168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSentinel-based MIN/MAX update does not set
has_minsflag.This sentinel-based approach correctly uses
std::min/std::maxwithout branching on first value, but never setsbucket.has_mins[i] = trueafter a non-NULL update. This breaks:
- Parallel aggregation:
merge_fromcheckshas_minsand skips merging (critical issue raised above).- Groups with all-NULL aggregate columns: Output phase will emit sentinel values (
INT64_MAXfor MIN,INT64_MINfor MAX) instead of NULL.Add
bucket.has_mins[i] = true;(for int64) andbucket.has_float_minmax[i] = true;(for float64) after thestd::min/std::maxupdates:bucket.mins_float64[i] = std::min(bucket.mins_float64[i], val); bucket.maxes_float64[i] = std::max(bucket.maxes_float64[i], val); + bucket.has_float_minmax[i] = true; } else { auto val = col.get(row_idx).to_int64(); bucket.mins[i] = std::min(bucket.mins[i], val); bucket.maxes[i] = std::max(bucket.maxes[i], val); + bucket.has_mins[i] = true; }🤖 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/vectorized_operator.hpp` around lines 1150 - 1168, The MIN/MAX sentinel update block for AggregateType::Min/Max updates bucket.mins/bucket.maxes and bucket.mins_float64/bucket.maxes_float64 but never marks the corresponding presence flags, which breaks merge_from and null-output handling; after performing the std::min/std::max update for the int64 path set bucket.has_mins[i] = true, and after the float64 path set bucket.has_float_minmax[i] = true so the bucket correctly records that a non-NULL value has been seen.
🧹 Nitpick comments (1)
CMakeLists.txt (1)
199-200: ⚡ Quick winAdd Homebrew search paths for macOS consistency.
The DuckDB benchmark (lines 187-188) includes
/opt/homebrew/liband/opt/homebrew/includein the search paths for macOS Homebrew installations. For consistency and improved portability, consider adding the same paths here.🔧 Suggested enhancement
- find_library(PQLIB_LIBRARY pq PATHS /usr/lib /usr/local/lib) - find_path(PQLIB_INCLUDE_DIR libpq-fe.h PATHS /usr/include /usr/local/include) + find_library(PQLIB_LIBRARY pq PATHS /opt/homebrew/lib /usr/lib /usr/local/lib) + find_path(PQLIB_INCLUDE_DIR libpq-fe.h PATHS /opt/homebrew/include /usr/include /usr/local/include)🤖 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 `@CMakeLists.txt` around lines 199 - 200, The find commands for libpq currently search only /usr/lib and /usr/local/lib for find_library(PQLIB_LIBRARY) and /usr/include and /usr/local/include for find_path(PQLIB_INCLUDE_DIR); update these invocations to also include macOS Homebrew locations by adding /opt/homebrew/lib to the PATHS for find_library and /opt/homebrew/include to the PATHS for find_path so Homebrew-installed PostgreSQL is discovered consistently (update the lines containing find_library(PQLIB_LIBRARY pq PATHS ...) and find_path(PQLIB_INCLUDE_DIR libpq-fe.h PATHS ...)).
🤖 Prompt for all review comments with 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.
Inline comments:
In `@benchmarks/postgresql_comparison_bench.cpp`:
- Around line 148-189: The INSERT benchmarks (BM_PostgreSQL_Insert and
BM_CloudSQL_Insert) append rows every iteration because the table is created
once and never cleared; fix by clearing the lineitem table at the start of each
benchmark iteration or document intent. Modify BM_PostgreSQL_Insert to call
ctx.execute_sql("TRUNCATE TABLE lineitem") (or DELETE FROM lineitem) immediately
inside the for (auto _ : state) loop before "BEGIN" (keeping ctx.create_tables()
once before the loop). Modify BM_CloudSQL_Insert similarly: call
ctx.executor->execute("TRUNCATE TABLE lineitem") (or the appropriate ParseSQL
variant) at the top of its per-iteration loop before "BEGIN". Alternatively, if
growth is intentional, add a clear comment in both BM_PostgreSQL_Insert and
BM_CloudSQL_Insert stating the benchmark measures inserts into a growing table.
- Around line 64-78: In create_tables(), each PQexec call currently discards the
PGresult* and ignores errors; capture each call's PGresult* (e.g., r =
PQexec(conn, "...")), check PQresultStatus(r) for PGRES_COMMAND_OK (or
appropriate success codes), handle/report failures (fprintf(stderr, ...) or
throw/exit) and always call PQclear(r) after checking; apply this pattern to the
SET, DROP TABLE and CREATE TABLE statements so no PGresult* is leaked and errors
are surfaced.
In `@docs/performance/POSTGRESQL_COMPARISON.md`:
- Line 163: Update the broken relative link in the Markdown line containing
"cloudSQL: [GitHub Repository](../README.md)" so it points to the repository
root README; replace "(../README.md)" with "(../../README.md)" to correctly
navigate up two levels to the top-level README.
- Around line 30-44: The docs declare PRIMARY KEY on l_orderkey and o_orderkey
in the CREATE TABLE examples but the benchmark's actual table-creation SQL for
the lineitem and orders tables does not add those constraints; fix by either (A)
updating the documentation to remove the PRIMARY KEY annotations for
lineitem.l_orderkey and orders.o_orderkey so the docs match the current
implementation, or (B) updating the implementation's CREATE TABLE statements for
the lineitem and orders tables to add "PRIMARY KEY" to l_orderkey and o_orderkey
respectively (and ensure any required index/constraint handling in the setup
code is adjusted accordingly) so the code matches the docs.
In `@include/executor/vectorized_operator.hpp`:
- Around line 533-539: The sentinel initialization disables MIN/MAX merging
because has_mins remains false; fix by setting has_mins[a] = true when
initializing bucket.mins/maxes/mins_float64/maxes_float64 (in the block with
bucket.mins[a] = std::numeric_limits<int64_t>::max()), and also update
update_bucket_accumulators to set has_mins[i] = true (and has_float_minmax[i] =
true for floating updates) whenever a non-NULL value updates a per-bucket min or
max; this preserves the existing merge_from logic (which checks src.has_mins[i])
and ensures thread-local buckets are merged correctly.
---
Outside diff comments:
In `@include/executor/vectorized_operator.hpp`:
- Around line 1170-1234: The batch update logic in update_aggregate_batch
incorrectly places the AggregateType::Min/Max handling after the
TYPE_INT64/TYPE_FLOAT64 branches so numeric MIN/MAX never run; change the
control flow so that agg.type is checked first for MIN/MAX (using
aggregates_[agg_idx].type / agg.type) and then, inside that branch, dispatch on
col.type() to update bucket.mins/mins_float64 and bucket.maxes/maxes_float64
(using NumericVector<int64_t>/NumericVector<double> and raw_data()), or
alternatively keep the numeric-type branches but branch on agg.type inside them
to perform sum (bucket.sums_int64/sums_float64 and bucket.has_float_value) for
non-min/max and min/max updates for AggregateType::Min/Max; ensure Count and
input_col_idx handling remains unchanged.
- Around line 1150-1168: The MIN/MAX sentinel update block for
AggregateType::Min/Max updates bucket.mins/bucket.maxes and
bucket.mins_float64/bucket.maxes_float64 but never marks the corresponding
presence flags, which breaks merge_from and null-output handling; after
performing the std::min/std::max update for the int64 path set
bucket.has_mins[i] = true, and after the float64 path set
bucket.has_float_minmax[i] = true so the bucket correctly records that a
non-NULL value has been seen.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 199-200: The find commands for libpq currently search only
/usr/lib and /usr/local/lib for find_library(PQLIB_LIBRARY) and /usr/include and
/usr/local/include for find_path(PQLIB_INCLUDE_DIR); update these invocations to
also include macOS Homebrew locations by adding /opt/homebrew/lib to the PATHS
for find_library and /opt/homebrew/include to the PATHS for find_path so
Homebrew-installed PostgreSQL is discovered consistently (update the lines
containing find_library(PQLIB_LIBRARY pq PATHS ...) and
find_path(PQLIB_INCLUDE_DIR libpq-fe.h PATHS ...)).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19f084b0-3cac-48c6-a7d6-011a8d422c9a
📒 Files selected for processing (4)
CMakeLists.txtbenchmarks/postgresql_comparison_bench.cppdocs/performance/POSTGRESQL_COMPARISON.mdinclude/executor/vectorized_operator.hpp
…gs, Homebrew paths - Fix PGresult* leak in create_tables(): check PQresultStatus and always PQclear - Fix INSERT benchmark: add TRUNCATE at start of each iteration for fair measurement - Fix MIN/MAX presence flags: set has_mins/has_float_minmax in update_bucket_accumulators - Add Homebrew paths for macOS PostgreSQL discovery in CMakeLists.txt - Fix broken link in POSTGRESQL_COMPARISON.md (../README.md -> ../../README.md)
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
1 participant
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.
Summary
Performance Impact
Summary by CodeRabbit
New Features
Documentation
Performance