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.22% 88.62% -0.60%
==========================================
Files 17 26 +9
Lines 4556 7878 +3322
Branches 828 1304 +476
==========================================
+ Hits 4065 6982 +2917
- Misses 284 504 +220
- Partials 207 392 +185
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 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 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f499afec2a
ℹ️ 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,3 @@ | |||
| [submodule "agentic/cpp"] | |||
| path = agentic/cpp | |||
| url = git@github.com:Malkovsky/ai_for_cpp.git | |||
There was a problem hiding this comment.
When a fresh clone or CI job initializes submodules without a GitHub SSH key, this git@github.com: URL fails with public-key authentication before agentic/cpp is populated. Since this commit replaces the previously tracked agentic/cpp files with a submodule that AGENTS.md points agents to read, use an anonymous HTTPS URL for the public submodule so default git submodule update --init works.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: caed4ffcda
ℹ️ 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".
| "${BUILD_DIR}/louds_tree_tests" | ||
| "${BUILD_DIR}/dfuds_tree_tests" | ||
| "${BUILD_DIR}/test_rmm" | ||
| "${BUILD_DIR}/rmq_tests" |
There was a problem hiding this comment.
When scripts/coverage_report.sh is run from a clean tree, cmake --build --preset coverage builds only the targets listed in the coverage build preset in CMakePresets.json (unittests, benchmark_tests, test_rmm, louds_tree_tests, dfuds_tree_tests, and excess_positions_tests). Since rmq_tests is not in that preset, this newly added invocation exits with “No such file or directory” before coverage is generated; add rmq_tests to the coverage preset or build it explicitly here before executing it.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
