Few implementations of RMQ#52
Conversation
Malkovsky
commented
May 30, 2026
Malkovsky
commented
- Implemented simple RMQ baselines: segment tree, sparse table
- Implemented cartesian tree reduction to RMQ +-1 and its base implementation via sparse table (aka FCB algorithm)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d00344a8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| append_euler(node, depth); | ||
| } | ||
| if (right_child_[node] != invalid_index) { | ||
| euler_tour(right_child_[node], depth + 1); |
There was a problem hiding this comment.
For monotone inputs the Cartesian tree is a chain, so this recursive call nests once per array element; constructing CartesianTreeRmq on a sorted array with a few hundred thousand elements overflows the default Linux stack (I reproduced a segfault with 500k ascending ints) even though the data size is otherwise reasonable for this library. An iterative Euler traversal avoids crashing on these common worst-case inputs.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,308 @@ | |||
| #include <gtest/gtest.h> | |||
There was a problem hiding this comment.
This new test source is never built or run: I checked the PIXIE_TESTS section of the top-level CMakeLists.txt, and it registers unittests, benchmark_tests, test_rmm, louds_tree_tests, and excess_positions_tests, but not src/tests/rmq_tests.cpp. As a result CI will not compile the new RMQ coverage or catch regressions in these new implementations unless an add_executable/gtest_discover_tests entry is added.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 89.47% 89.98% +0.51%
==========================================
Files 15 15
Lines 4416 4685 +269
Branches 814 832 +18
==========================================
+ Hits 3951 4216 +265
- Misses 280 284 +4
Partials 185 185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5357398423
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,188 @@ | |||
| #include <benchmark/benchmark.h> | |||
There was a problem hiding this comment.
The new benchmark source is never built: I checked the PIXIE_BENCHMARKS section of the top-level CMakeLists.txt, and it registers targets such as benchmarks, bench_rmm, bench_rmm_btree, louds_tree_benchmarks, and excess_positions_benchmarks, but not src/benchmarks/bench_rmq.cpp. This means the RMQ benchmark coverage added here cannot be run through the normal CMake build, so performance experiments for these new implementations silently miss the benchmark unless a target/link entry is added.
Useful? React with 👍 / 👎.
| bench_name = parts[0] | ||
| try: | ||
| n = int(parts[1]) | ||
| except ValueError: |
There was a problem hiding this comment.
For multi-argument Google Benchmark names such as the RMQ benchmarks registered as rmq_sparse_table/<size>/<width>, this parses only parts[1] and later keys series by (bench_name, n), so all widths for the same size get averaged together. That makes the plotted RMQ results misleading because max_width is intentionally varied in bench_rmq.cpp; include the remaining arguments in the series key or otherwise split them instead of collapsing them into one point.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3bf81cdb0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const std::vector<std::size_t> sizes = {1ull << 10, 1ull << 14, 1ull << 18, | ||
| 1ull << 22, 1ull << 26}; |
There was a problem hiding this comment.
When bench_rmq is run without a filter, this size list is used for every RMQ registered in the loop, including rmq_sparse_table in the same section. At 1ull << 26, SparseTable allocates O(n log n) stored indices (about 1.7B size_t entries, roughly 14 GiB before the input/query vectors), so the normal benchmark target will OOM or spend its time swapping before producing results. Please keep the very large sizes out of the sparse-table baseline or register per-implementation size sets.
Useful? React with 👍 / 👎.
1 participant
