Conversation
ti-chi-bot
bot
added
the
release-note-none
|
[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 |
Review Completed ✅Status: 4 issues identified and posted as PR review comments Findings Summary:
Review Links:
Issues Found:
Recommendation: Address the P0 and P1 issues before merging to ensure the code builds successfully. |
ti-chi-bot
bot
added
the
size/XL
📝 WalkthroughWalkthroughThis PR introduces support for a hidden commit timestamp column (ID -5) by adding aliasing and casting logic across the TiFlash query execution pipeline. The column ID is mapped to the internal version column throughout storage access, filtering, and expression analysis components, with comprehensive tests validating the aliasing and type casting behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@dbms/src/Flash/Coprocessor/GenSchemaAndColumn.cpp`:
- Around line 136-138: The throw in the switch case for
MutSup::extra_commit_ts_col_id in GenSchemaAndColumn.cpp omits an ErrorCodes
value; update the throw to include a DB::ErrorCodes enum (e.g.,
ErrorCodes::NOT_IMPLEMENTED) so it follows the pattern throw Exception("Not
supported in disaggregated read now", ErrorCodes::NOT_IMPLEMENTED); — locate the
case handling MutSup::extra_commit_ts_col_id and replace the current throw
Exception(...) with one that passes the appropriate ErrorCodes::... constant.
In `@dbms/src/Storages/MutableSupport.h`:
- Line 46: Rename the snake_case constant extra_commit_ts_col_id to camelCase
extraCommitTsColId and update every usage and reference accordingly; locate the
declaration inline static constexpr ColumnID extra_commit_ts_col_id in
MutableSupport.h and change the identifier, then update all call sites, static
casts, comments, and any tests or documentation that use extra_commit_ts_col_id
so they reference extraCommitTsColId to comply with the project's naming
guideline.
In `@dbms/src/Storages/tests/gtest_hidden_commit_ts_column.cpp`:
- Line 46: Update the inline comment that reads "Maybe another test has already
registed, ignore exception here." to correct the typo: change "registed" to
"registered" so it reads "Maybe another test has already registered, ignore
exception here." — edit this comment in gtest_hidden_commit_ts_column.cpp (the
comment string above the exception-handling block) to fix the spelling only.
| case MutSup::extra_commit_ts_col_id: | ||
| throw Exception("Not supported in disaggregated read now"); | ||
| default: |
There was a problem hiding this comment.
Add an ErrorCodes value to this Exception.
The current throw omits an error code, which makes error handling inconsistent.
🔧 Suggested fix
- case MutSup::extra_commit_ts_col_id:
- throw Exception("Not supported in disaggregated read now");
+ case MutSup::extra_commit_ts_col_id:
+ throw Exception("Not supported in disaggregated read now", ErrorCodes::NOT_IMPLEMENTED);🤖 Prompt for AI Agents
In `@dbms/src/Flash/Coprocessor/GenSchemaAndColumn.cpp` around lines 136 - 138,
The throw in the switch case for MutSup::extra_commit_ts_col_id in
GenSchemaAndColumn.cpp omits an ErrorCodes value; update the throw to include a
DB::ErrorCodes enum (e.g., ErrorCodes::NOT_IMPLEMENTED) so it follows the
pattern throw Exception("Not supported in disaggregated read now",
ErrorCodes::NOT_IMPLEMENTED); — locate the case handling
MutSup::extra_commit_ts_col_id and replace the current throw Exception(...) with
one that passes the appropriate ErrorCodes::... constant.
|
|
||
| inline static constexpr ColumnID extra_handle_id = -1; | ||
| inline static constexpr ColumnID extra_table_id_col_id = -3; | ||
| inline static constexpr ColumnID extra_commit_ts_col_id = -5; |
There was a problem hiding this comment.
Rename to camelCase to match naming guideline.
extra_commit_ts_col_id introduces a new snake_case identifier.
🔧 Suggested rename (plus update all usages)
- inline static constexpr ColumnID extra_commit_ts_col_id = -5;
+ inline static constexpr ColumnID extraCommitTsColId = -5;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| inline static constexpr ColumnID extra_commit_ts_col_id = -5; | |
| inline static constexpr ColumnID extraCommitTsColId = -5; |
🤖 Prompt for AI Agents
In `@dbms/src/Storages/MutableSupport.h` at line 46, Rename the snake_case
constant extra_commit_ts_col_id to camelCase extraCommitTsColId and update every
usage and reference accordingly; locate the declaration inline static constexpr
ColumnID extra_commit_ts_col_id in MutableSupport.h and change the identifier,
then update all call sites, static casts, comments, and any tests or
documentation that use extra_commit_ts_col_id so they reference
extraCommitTsColId to comply with the project's naming guideline.
| } | ||
| catch (DB::Exception &) | ||
| { | ||
| // Maybe another test has already registed, ignore exception here. |
There was a problem hiding this comment.
Typo: "registed" → "registered".
- // Maybe another test has already registed, ignore exception here.
+ // Maybe another test has already registered, ignore exception here.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Maybe another test has already registed, ignore exception here. | |
| // Maybe another test has already registered, ignore exception here. |
🤖 Prompt for AI Agents
In `@dbms/src/Storages/tests/gtest_hidden_commit_ts_column.cpp` at line 46, Update
the inline comment that reads "Maybe another test has already registed, ignore
exception here." to correct the typo: change "registed" to "registered" so it
reads "Maybe another test has already registered, ignore exception here." — edit
this comment in gtest_hidden_commit_ts_column.cpp (the comment string above the
exception-handling block) to fix the spelling only.
|
@xzhangxian1008: The following tests 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. |
![pantheon-ai[bot]](https://avatars.githubusercontent.com/in/2265341?s=60&v=4){kind=small}
| has_cast = true; | ||
| } | ||
|
|
||
| if (may_need_add_cast_column[i] && table_scan_columns[i].id == ExtraCommitTSColumnID) |
There was a problem hiding this comment.
P0 - Undefined ExtraCommitTSColumnID breaks build
ExtraCommitTSColumnID is referenced but not defined anywhere in the codebase, causing build failure.
Evidence: This file is part of the default flash_service library build. A repo-wide search shows only uses (no definition) at line 1261 and in test file.
Likely fix: Use MutSup::extra_commit_ts_col_id (defined in Storages/MutableSupport.h:46).
| ColumnID col_id = col_info.id; | ||
| // TiDB may request a hidden commit_ts column in TableScan with a special ColumnID. | ||
| // In TiFlash it is stored in `_INTERNAL_VERSION` (VersionColumnID), so create an alias mapping. | ||
| if (unlikely(col_id == MutSup::extra_commit_ts_col_id)) |
There was a problem hiding this comment.
P2 - Rough set filter commit_ts alias uses wrong map key
RSOperator::build intends to alias TiDB's hidden commit_ts ColumnID (-5) to TiFlash's internal version ColumnID (-1024), but only populates the map under key -1024. Later, FilterParser resolves ColumnRef to the original ColumnID (-5) and calls id_to_attr.at(-5), which will throw std::out_of_range if a filter references commit_ts.
Evidence: Line 76 rewrites col_id from -5 to -1024, but map insertion at line 89 uses column_id_to_attr[cd.id], creating key -1024 only. Later lookup at FilterParser.cpp:168 uses the original -5.
Suggested fix: Populate the map under both keys, or adjust the lookup to use the aliased key.
| #include <IO/Buffer/WriteBufferFromString.h> | ||
| #include <Interpreters/Context.h> | ||
| #include <Storages/DeltaMerge/DeltaMergeDefines.h> | ||
| #include <Storages/DeltaMerge/Filter/PushDownFilter.h> |
There was a problem hiding this comment.
P1 - New gtest does not compile (multiple issues)
This test file has several compilation errors that block the gtests_dbms build:
- Line 23: Includes non-existent header
<Storages/DeltaMerge/Filter/PushDownFilter.h>(directory containsPushDownExecutor.*instead) - Lines 63, 92, 94, 225: References undefined constants
ExtraCommitTSColumnID,VERSION_COLUMN_ID,VERSION_COLUMN_NAME,VERSION_COLUMN_TYPE, and non-existentDM::PushDownFilter::build(...)API - Line 215: Calls
DAGQueryInfoconstructor with wrong signature (missingfts_query_infoparameter)
Evidence: Current code uses DB::MutSup::{extra_commit_ts_col_id, version_col_id, version_column_name, getVersionColumnType()} and DM::getVersionColumnDefine() instead.
|
@pantheon-ai[bot]: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
| break; | ||
| } | ||
| case MutSup::extra_commit_ts_col_id: | ||
| throw Exception("Not supported in disaggregated read now"); |
There was a problem hiding this comment.
P2 - Disaggregated read rejects commit_ts
Disaggregated compute mode's TableScan path unconditionally throws when it encounters the hidden commit_ts ColumnID (MutSup::extra_commit_ts_col_id, -5), so any DAG request that includes that column will fail in disaggregated mode.
Evidence:
- Line 137:
case MutSup::extra_commit_ts_col_id: throw Exception(\"Not supported in disaggregated read now\"); - Called from
StorageDisaggregatedRemote.cpp:759and:833 - Non-disaggregated paths map -5 to
_INTERNAL_VERSIONinstead of rejecting it
Suggested fix: Implement the same aliasing behavior (-5 → version column) for disaggregated mode instead of throwing.
1 participant
What problem does this PR solve?
Issue Number: ref #10715
Problem Summary:
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
New Features
Tests