ddl: Fix default value filling with finer granularity (#10682)#10690
ddl: Fix default value filling with finer granularity (#10682)#10690ti-chi-bot wants to merge 7 commits intopingcap:release-7.5from
Conversation
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
ti-chi-bot
added
do-not-merge/hold
|
This cherry pick PR is for a release branch and has not yet been approved by triage owners. To merge this cherry pick:
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. |
|
@JaySon-Huang This PR has conflicts, I have hold it. |
|
@ti-chi-bot: ## If you want to know how to resolve it, please read the guide in TiDB Dev Guide. 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 ti-community-infra/tichi repository. |
📝 WalkthroughWalkthroughCentralized row-decoding now backfills missing columns using origin-default/default/NULL rules or requests a schema sync via a new helper; adds ColumnInfo helpers for origin-default access and safe parsing; documents RegionBlockReader decoding positions; and extends unit and full-stack tests covering nullability and default-filling behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Decoder as Row Decoder
participant Helper as addDefaultValueToColumnIfPossible
participant ColInfo as ColumnInfo
participant Block as Block
Decoder->>Decoder: parse encoded row, detect column id mismatch
Decoder->>Helper: request fill(column_info, block_pos, force_decode, ignore_pk_if_absent)
activate Helper
Helper->>ColInfo: hasOriginDefaultValue()
ColInfo-->>Helper: bool
alt origin-default present
Helper->>ColInfo: defaultValueToField()
ColInfo-->>Helper: Field
Helper->>Block: insert Field at block_pos
Helper-->>Decoder: success
else origin-default absent
alt nullable or force_decode permits
Helper->>Block: insert NULL or generated default
Helper-->>Decoder: success
else cannot fill
Helper-->>Decoder: failure (request schema sync)
end
end
deactivate Helper
Decoder->>Decoder: continue decoding or trigger schema sync
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 |
![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/Storages/KVStore/Decode/RegionBlockReader.cpp`:
- Around line 114-151: The file contains unresolved merge conflict markers that
inserted a VersionColResolver<RegionUncommittedDataList> specialization and a
stray "private:" block inside RegionBlockReader::readImpl, breaking compilation;
move the entire VersionColResolver<RegionUncommittedDataList> struct out of the
readImpl function scope to namespace/global scope and remove the conflict
markers, then restore readImpl to use the correct column-id mapping (replace the
residual sorted_column_id_with_pos with schema_snapshot->getColId2BlockPosMap()
as used in the incoming change) so VersionColResolver,
RegionUncommittedDataList, and RegionBlockReader::readImpl are correctly defined
and compile.
In `@dbms/src/TiDB/Decode/RowCodec.cpp`:
- Around line 418-424: Resolve the leftover Git conflict markers inside
addDefaultValueToColumnIfPossible by removing the <<<<<<<, =======, and >>>>>>>
lines and consolidating the two comment variants into a single coherent comment
(e.g., "fallthrough for best-effort fill when force_decode == true" or the
longer explanation about filling primary-key-flagged columns for older schemas)
so the file compiles; ensure only one comment remains above the fallthrough
logic and no conflict markers remain.
In `@dbms/src/TiDB/Schema/tests/gtest_table_info.cpp`:
- Around line 140-170: Remove the Git conflict markers and keep the new
ParseCase tests that validate origin_default parsing: delete the lines with
<<<<<<< HEAD, =======, and >>>>>>> 01b12dd900 and ensure both ParseCase blocks
(the ones asserting col_3.defaultValueToField() == "100" and == "200") remain
intact; verify the tests still reference TableInfo, getColumnInfo(3), and the
assertions on col_3.tp, hasNotNullFlag(), hasOriginDefaultValue(), and
defaultValueToField() so the origin_default parsing tests for issue `#10663`
compile and run.
🧹 Nitpick comments (1)
dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp (1)
694-866: Comprehensive test coverage for default value handling in region decoding.This test thoroughly validates the fix for issue
#10680by covering all key scenarios:
- NOT NULL columns without origin_default (schema sync required)
- NOT NULL columns with origin_default (can decode without force)
- NOT NULL columns with non-public state (schema sync required even with origin_default)
- Nullable columns (NULL preserved for missing values)
The test correctly verifies both
force_decode=falseandforce_decode=truepaths for each scenario.Minor: Lines 818 and 844 have a typo "thrid" → "third" in comments.
✏️ Fix typos in comments
// the thrid elem is filled wih origin_default + // the third elem is filled with origin_default(Apply similar fix at line 844)
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp`:
- Around line 818-819: Fix the typo in the comment above the createColumn<Int64>
call: change "thrid" to "third" (and correct "wih" to "with" if present) so the
comment reads e.g. "the third elem is filled with origin_default" near the
createColumn<Int64>({-2051270087, -2051270087, -56083770})); this touches the
comment text only—no code changes required.
- Around line 844-845: Fix the typo in the inline comment next to the
createColumn<Int64> call in the gtest_region_block_reader test: change "thrid
elem is filled wih origin_default" to "third elem is filled with origin_default"
near the createColumn<Int64>({-2051270087, -2051270087, -56083770}) invocation
so the test comment reads correctly.
🧹 Nitpick comments (1)
dbms/src/TiDB/Schema/tests/gtest_table_info.cpp (1)
140-167: Good test coverage for origin_default parsing.These new ParseCase tests properly validate the
hasOriginDefaultValue()anddefaultValueToField()methods for the fix described in issue#10663.Note: The second ParseCase (lines 155-167) is missing the
ASSERT_TRUE(col_3.hasOriginDefaultValue())assertion that the first case has at line 151. Consider adding it for consistency.📝 Suggested addition for consistency
auto col_3 = table_info.getColumnInfo(3); ASSERT_EQ(col_3.tp, TiDB::TP::TypeVarchar); ASSERT_TRUE(col_3.hasNotNullFlag()); + ASSERT_TRUE(col_3.hasOriginDefaultValue()); // The "origin_default" is "200", which is used for filling default value for old rows that is inserted before this column is added. ASSERT_EQ(col_3.defaultValueToField().get<String>(), "200");
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp`:
- Around line 762-799: The comment and assertion conflict: for the schema
referenced by
getDecodingStorageSchemaSnapshot(table_info_c1_not_null_no_origin_default) the
stored origin default is "-56083770" but the test asserts 0; decide which
behavior is intended and make them consistent—if the implementation
intentionally fills a zero fallback for missing not-null defaults, update the
comment near the RegionBlockReader/read test to say “fills zero fallback for
missing not-null default” and keep ASSERT_COLUMN_EQ against
createColumn<Int64>({0,...}) (adjust values accordingly); otherwise update the
ASSERT_COLUMN_EQ to expect createColumn<Int64>({-56083770,-56083770,0}) (or the
correct per-row default) and change the comment to say “fills the origin default
(-56083770) for c1.” Ensure changes reference the test using RegionBlockReader,
reader.read, and
getDecodingStorageSchemaSnapshot(table_info_c1_not_null_no_origin_default).
🧹 Nitpick comments (3)
dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp (3)
697-750: Rename new locals to camelCase to match style.These new locals are snake_case in a C++ test file (e.g.,
table_info_c1_not_null_no_origin_default,region_start_key,data_list_read). Please rename to camelCase to align with the codebase conventions. The snippet below shows one example; apply similarly to the other locals.🔧 Example rename
- TableInfo table_info_c1_not_null_no_origin_default( + TableInfo tableInfoC1NotNullNoOriginDefault( R"({"cols":[{"id":1,"name":{"L":"c0","O":"c0"},"offset":0,"state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":0,"Flag":0,"Flen":4,"Tp":1}},{"id":2,"name":{"L":"handle","O":"handle"},"offset":1,"state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":0,"Flag":515,"Flen":11,"Tp":3}},{"default":"-56083770","id":7,"name":{"L":"c1","O":"c1"},"offset":2,"state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":0,"Flag":1,"Flen":20,"Tp":8}},{"id":4,"name":{"L":"c2","O":"c2"},"offset":3,"origin_default":"0.07954397","state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":-1,"Flag":4097,"Flen":12,"Tp":4}},{"id":5,"name":{"L":"c5","O":"c5"},"offset":4,"origin_default":"0","state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":0,"Flag":0,"Flen":10,"Tp":246}},{"default":"247262911","id":6,"name":{"L":"c4","O":"c4"},"offset":5,"origin_default":"247262911","state":5,"type":{"Charset":"binary","Collate":"binary","Decimal":0,"Flag":0,"Flen":10,"Tp":246}}],"id":636,"index_info":[],"is_common_handle":false,"keyspace_id":4294967295,"name":{"L":"t0","O":"t0"},"pk_is_handle":true,"state":5,"tiflash_replica":{"Available":true,"Count":1},"update_timestamp":463845180343844895})", NullspaceID); @@ - auto decoding_schema = getDecodingStorageSchemaSnapshot(table_info_c1_not_null_no_origin_default); + auto decoding_schema = getDecodingStorageSchemaSnapshot(tableInfoC1NotNullNoOriginDefault);As per coding guidelines, Method and variable names in C++ must use camelCase (e.g., readBlock, totalBytes).
744-747: PreferColumnFamilyType::Writeover string literal.Using a string literal here is more error‑prone and inconsistent with the earlier test in this file. Consider using the enum to keep it type‑safe.
♻️ Suggested change
- region->insert("write", TiKVKey(bytesFromHexString(k)), TiKVValue(bytesFromHexString(v))); + region->insert(ColumnFamilyType::Write, TiKVKey(bytesFromHexString(k)), TiKVValue(bytesFromHexString(v)));
855-858: Use the fixture logger and add context to decoded‑block logs.This test already has a
LoggerPtr loggermember; using it and addingregion_idimproves consistency and traceability. Apply similarly to the other LOG_INFO calls in this test.🔍 Example adjustment
- LOG_INFO( - Logger::get(), - "Decoded block:\n{}", - DB::tests::getColumnsContent(res_block.getColumnsWithTypeAndName())); + LOG_INFO( + logger, + "Decoded block for region_id={}:\n{}", + region_id, + DB::tests::getColumnsContent(res_block.getColumnsWithTypeAndName()));As per coding guidelines, Use
LoggerPtrandLOG_INFO(log, ...)with relevant context (e.g.,region_id,table_id) for logging in storage engine code.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@format-diff.py`:
- Around line 32-39: The try_find_clang_format function currently only tries
'clang-format-13' and can return a non-existent binary; update
try_find_clang_format to include 'clang-format' as a fallback candidate (e.g.,
candidates = ['clang-format-13', 'clang-format'] and still honor exec_path at
front) and return None if no executable is found. Then update
get_clang_format_version to check the result of try_find_clang_format (or accept
None) and raise a clear FileNotFoundError with a descriptive message if no
clang-format binary is found instead of attempting to execute a missing binary.
Ensure you reference try_find_clang_format and get_clang_format_version when
making these changes.
JaySon-Huang
force-pushed
the
cherry-pick-10682-to-release-7.5
branch
from
d590bf3 to
3ab5a7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@format-diff.py`:
- Around line 42-45: The function get_clang_format_version calls subprocess.run
but doesn't capture stdout, so proc.stdout is None and causes an AttributeError;
update the subprocess.run call in get_clang_format_version to capture output
(e.g., pass capture_output=True or stdout=subprocess.PIPE and text=True) and
then use proc.stdout.strip(); also handle subprocess.CalledProcessError by
either re-raising or logging a clear error so failures to run the clang-format
command are surfaced.
🧹 Nitpick comments (1)
format-diff.py (1)
102-108: Avoid shell concatenation when invoking clang-format.Now that
--clang_formatis user-supplied, building a shell string can break on paths with spaces and invites quoting issues. Prefer list args andshell=False.♻️ Suggested refactor
- cmd = clang_format_cmd + ' -i {}'.format(file) - if subprocess.Popen(cmd, shell=True, cwd=tiflash_repo_path).wait(): + cmd = [clang_format_cmd, '-i', file] + if subprocess.Popen(cmd, cwd=tiflash_repo_path).wait(): exit(-1)
JaySon-Huang
force-pushed
the
cherry-pick-10682-to-release-7.5
branch
2 times, most recently
from
6f83936 to
c2de343
Compare
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
JaySon-Huang
force-pushed
the
cherry-pick-10682-to-release-7.5
branch
from
11deacf to
fd83fae
Compare
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
2 participants
This is an automated cherry-pick of #10682
What problem does this PR solve?
Issue Number: close #10680
Problem Summary:
Root cause
What is changed and how it works?
With old schema (NOT NULL, no origin default), missing column now triggers schema sync. After schema sync, column becomes nullable, so missing column can be decode and stored in tiflash with
NULL, matching TiKV.Check List
Tests
Side effects
Documentation
Release note
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores