columnar: fix BIGINT clustered PK at INT64_MAX missing on TiFlash read#10869
Conversation
Pin kvengine and related crates to the i64::MAX columnar read fix, and document how to maintain the dependency.
Embed kvengine git rev from Cargo.lock in version output and log it after logger init.
|
@JaySon-Huang I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
📝 WalkthroughWalkthroughThis PR introduces build-time capture of cloud-storage-engine git revision from Cargo.lock, refactors version reporting to use that captured hash, adds startup logging of version details, and documents the columnar hub architecture and dependency management workflow. ChangesColumnar Hub Dependency Versioning
Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The PR involves a new build script with parsing logic and multiple interconnected changes across build, runtime, and runtime initialization code. The hash extraction logic requires careful review of Cargo.lock format parsing and edge cases, while version info integration touches startup flow. Documentation is substantial but straightforward. Mixed complexity across distinct files warrants moderate review effort. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Guide agents to read .github templates when opening issues or pull requests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@contrib/tiflash-columnar-hub/hub-runtime/build.rs`:
- Around line 38-41: The current parsing logic in build.rs only checks for
"?rev=" and returns Unknown for branch-tracking entries; update the extraction
logic around source.find("?rev=")/rev_part to also detect "?branch=" and, when a
branch param is present, parse the string after the '#' character (the resolved
commit hash) instead of the branch name; modify the code that computes
rev_part/hash (used where rev_start and rev_part are referenced) to check for
"?branch=" if "?rev=" is absent, split on '#' and return the post-# token
trimmed of quotes as the commit hash.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a482af76-59aa-41ef-a106-d7c8f12bbb3e
⛔ Files ignored due to path filters (1)
contrib/tiflash-columnar-hub/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
contrib/tiflash-columnar-hub/AGENTS.mdcontrib/tiflash-columnar-hub/Cargo.tomlcontrib/tiflash-columnar-hub/hub-runtime/build.rscontrib/tiflash-columnar-hub/hub-runtime/src/lib.rscontrib/tiflash-columnar-hub/hub-runtime/src/run.rs
|
@JaySon-Huang: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Use branch = "cloud-engine" in Cargo.toml while keeping Cargo.lock at a9d93252f, parse git hash from branch-style lock entries, and move startup version logging into run.rs.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JinheLin, yongman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #10852
Problem Summary:
On next-gen / disaggregated columnar read path, queries succeed but omit the row whose clustered BIGINT PK is
9223372036854775807(i64::MAX). For example,SELECT * FROM t WHERE a > INT64_MINreturns only the0row on TiFlash instead of both0andINT64_MAX. Full table scan andCOUNT(*)also report 2 rows instead of 3.Root cause is in
cloud-storage-engine/kvengine: when building columnar handle index,next_handle + 1overflows ati64::MAX, so no sentinel is appended and the last pack cannot be read via seek.What is changed and how it works?
cloud-storage-engine(kvengine and related crates) to revisiona9d93252f, which fixes handle-index sentinel handling when max clustered PK isi64::MAX.contrib/tiflash-columnar-hub/AGENTS.mddocumenting how to maintain the columnar-hub dependency.Cargo.lockin version output and log it at startup for easier on-cluster debugging.After deploying, rebuild/restart TiFlash and rebuild or re-compact columnar files for affected tables before re-running SQL verification.
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Release Notes
Documentation
Improvements