Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
storage/eloq/ha_eloq.h (1)
718-739: Aggressive prefetch when an end key is present – consider de‑magicifying 256.Using a fixed
return 256;whenend_specified_is true is straightforward and aligns with the intent to prefetch more aggressively on bounded scans, but it does hard‑code a new behavior step change. Consider:
- Defining a named constant (e.g.
static constexpr uint32_t kBoundedScanPrefetch = 256;) nearDEFAULT_SCAN_TUPLE_SIZEto document and centralize the tuning knob.- Optionally revisiting the value in perf tests to ensure small bounded ranges are not over‑prefetched compared to the prior adaptive ramp‑up.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
data_substrate(1 hunks)storage/eloq/ha_eloq.cc(3 hunks)storage/eloq/ha_eloq.h(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-07T07:10:40.633Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.633Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
Applied to files:
storage/eloq/ha_eloq.ccstorage/eloq/ha_eloq.h
🔇 Additional comments (3)
data_substrate (1)
1-1: Submodule update aligns with PR objectives—verify no breaking changes indata_substrate.The
data_substratesubmodule pointer update to commitd2a3c09is part of the commit "prefetch more aggressively when end key is specified," which directly supports the PR's goal of optimizing large scan requests. Ensure the new submodule commit does not introduce breaking changes to dependent code.storage/eloq/ha_eloq.h (1)
781-787: end_specified_ scan state flag wiring looks correct.Default initialization to
falseand placement next toscan_end_key_/scan_end_tx_key_are appropriate for per‑scan state; the .cc changes set/clear it consistently on scan open/close.storage/eloq/ha_eloq.cc (1)
4123-4166: end_specified_ lifecycle around scan open/close is coherent.Setting
end_specified_inPkIndexScanOpen/SkIndexScanOpenbased onend_key/end_index_keynullness and clearing it inIndexScanClose()cleanly scopes the flag to the lifetime of a single open scan and matches howPrefetchSize()is consulted in the scan batch paths. Given current call sites only pass a non‑null end pointer when a real end boundary has been computed, this looks functionally sound.Also applies to: 4188-4227, 5158-5219
Summary by CodeRabbit
Chores
Bug Fixes / Performance
✏️ Tip: You can customize this high-level summary in your review settings.