columnar: Fix partition table reading and columnar OOB crash on generated columns#10858
Open
JaySon-Huang wants to merge 4 commits into
Open
columnar: Fix partition table reading and columnar OOB crash on generated columns#10858JaySon-Huang wants to merge 4 commits into
JaySon-Huang wants to merge 4 commits into
Conversation
ti-chi-bot
Bot
added
the
release-note
|
@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. |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
the
size/M
📝 WalkthroughWalkthroughRemoves a redundant generated-column filter and reorders placeholder execution, aligns proxy operator headers with table-scan header, updates planner diagnostic messages/includes, and refines KV/Region restore behavior for columnar mode. ChangesGenerated Columns Disaggregated Read Fix
Sequence Diagram(s)sequenceDiagram
participant Client
participant PipelineExecutorContext
participant StorageDisaggregated
participant RNProxySourceOp
participant executeGeneratedColumnPlaceholder
Client->>PipelineExecutorContext: request readThroughColumnar(...)
PipelineExecutorContext->>StorageDisaggregated: invoke readThroughColumnar
StorageDisaggregated->>RNProxySourceOp: add proxy source operators (group_builder.addConcurrency)
StorageDisaggregated->>executeGeneratedColumnPlaceholder: compute generated_column_infos
StorageDisaggregated->>executeGeneratedColumnPlaceholder: executeGeneratedColumnPlaceholder(...)
executeGeneratedColumnPlaceholder-->>PipelineExecutorContext: return filled virtual/generated columns
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
JaySon-Huang
changed the title
ti-chi-bot
Bot
added
the
do-not-merge/work-in-progress
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes correctness and stability issues in the disaggregated next-gen columnar read path: (1) partition table scans needing _tidb_tid now keep stream headers consistent with the expected table-scan schema, and (2) a generated-column handling bug that could cause out-of-bounds access (manifesting as std::bad_alloc) is removed.
Changes:
- Align RN proxy stream/source headers with
AddExtraTableIDColumnTransformActionso_tidb_tidis present in headers for partition scans. - Remove the redundant generated-column filtering pass in
genColumnDefinesForDisaggregatedReadThroughColumnar()(generated columns are already excluded upstream and filled later). - Improve schema/type mismatch diagnostics in
PhysicalTableScanby including structured schema/header dumps.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dbms/src/Storages/StorageDisaggregatedColumnar.h | Use action.getHeader() to keep headers consistent when injecting _tidb_tid. |
| dbms/src/Storages/StorageDisaggregatedColumnar.cpp | Remove buggy redundant generated-column filtering; rely on existing skip + placeholder fill. |
| dbms/src/Flash/Planner/Plans/PhysicalTableScan.cpp | Enhance exception details for schema/type mismatches (adds JSON dumps). |
| dbms/src/Core/NamesAndTypes.h | Remove unused <map>/<set> includes (but see review comment: causes downstream compile break). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JaySon-Huang
force-pushed
the
fix/partition-table-and-columnar-oob
branch
from
3f8a1f0 to
2a2027f
Compare
Signed-off-by: JaySon-Huang <tshent@qq.com>
…loop. genColumnDefinesForDisaggregatedRead already excludes generated columns from column_defines. The second-pass loop incorrectly indexed column_defines with table_scan indices, causing misaligned columns and std::bad_alloc. Fixes pingcap#10856
Signed-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang
force-pushed
the
fix/partition-table-and-columnar-oob
branch
from
2a2027f to
e468a83
Compare
Signed-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang
changed the title
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dbms/src/Storages/StorageDisaggregatedColumnar.cpp (1)
533-536: ⚡ Quick winUse error-code-first
DB::Exceptionconstructor in these new throw paths.Please switch to the fmt-style
Exception(ErrorCodes::..., "msg {}", arg)form instead of building a string viafmt::formatfirst.Proposed fix
- throw Exception(fmt::format("pd client error: {}", error_msg), ErrorCodes::COLUMNAR_SNAPSHOT_ERROR); + throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "pd client error: {}", error_msg); ... - throw Exception( - fmt::format("columnar reader error_type={}, error={}", uint8_t(columnar_reader.error_type), error_msg), - ErrorCodes::COLUMNAR_SNAPSHOT_ERROR); + throw Exception( + ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, + "columnar reader error_type={}, error={}", + uint8_t(columnar_reader.error_type), + error_msg);As per coding guidelines: "Prefer the fmt-style constructor for
DB::Exceptionwith error code first:throw Exception(ErrorCodes::SOME_CODE, "Message with {}", arg);".Also applies to: 545-547
🤖 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 `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp` around lines 533 - 536, Replace the ad-hoc fmt::format-based Exception throws with the error-code-first DB::Exception constructor: instead of building error_msg and calling throw Exception(fmt::format(...), ErrorCodes::COLUMNAR_SNAPSHOT_ERROR), call throw Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "pd client error: {}", String(columnar_reader.error.buff.data, columnar_reader.error.buff.len)); do the same for the other occurrence around the columnar reader failure (also update the associated LOG_WARNING usage to keep the same formatted message but use the raw buffer string as the argument).
🤖 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.
Nitpick comments:
In `@dbms/src/Storages/StorageDisaggregatedColumnar.cpp`:
- Around line 533-536: Replace the ad-hoc fmt::format-based Exception throws
with the error-code-first DB::Exception constructor: instead of building
error_msg and calling throw Exception(fmt::format(...),
ErrorCodes::COLUMNAR_SNAPSHOT_ERROR), call throw
Exception(ErrorCodes::COLUMNAR_SNAPSHOT_ERROR, "pd client error: {}",
String(columnar_reader.error.buff.data, columnar_reader.error.buff.len)); do the
same for the other occurrence around the columnar reader failure (also update
the associated LOG_WARNING usage to keep the same formatted message but use the
raw buffer string as the argument).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 94cf982c-c896-45b1-8cce-f4ee4933785c
📒 Files selected for processing (6)
dbms/src/Core/NamesAndTypes.hdbms/src/Flash/Planner/Plans/PhysicalTableScan.cppdbms/src/Storages/KVStore/TMTContext.cppdbms/src/Storages/StorageDisaggregatedColumnar.cppdbms/src/Storages/StorageDisaggregatedColumnar.htests/fullstack-test-next-gen/placement/placement_in_sql.test
💤 Files with no reviewable changes (1)
- dbms/src/Core/NamesAndTypes.h
✅ Files skipped from review due to trivial changes (2)
- tests/fullstack-test-next-gen/placement/placement_in_sql.test
- dbms/src/Flash/Planner/Plans/PhysicalTableScan.cpp
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
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.
What problem does this PR solve?
Issue Number: close #10856
Problem Summary:
column_defineswithtable_scanindices. SincegenColumnDefinesForDisaggregatedReadalready excludes generated columns, this misalignment could cause out-of-bounds access andstd::bad_alloc.What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
Chores
Tests