Skip to content

[release-8.5] ddl: Fix default value filling with finer granularity (#10682)#10914

Open
3pointer wants to merge 3 commits into
pingcap:release-8.5-20260622-v8.5.4from
3pointer:codex/cherry-pick-10682-release-8.5.4
Open

[release-8.5] ddl: Fix default value filling with finer granularity (#10682)#10914
3pointer wants to merge 3 commits into
pingcap:release-8.5-20260622-v8.5.4from
3pointer:codex/cherry-pick-10682-release-8.5.4

Conversation

@3pointer

@3pointer 3pointer commented Jun 24, 2026

Copy link
Copy Markdown

What changed

Cherry-pick #10682 to release-8.5-20260622-v8.5.4.

This backports the default value filling fix so missing NOT NULL columns without origin default trigger schema sync instead of silently filling zero, keeping TiFlash results consistent with TiKV after altering a column from NOT NULL to NULL.

Notes

  • Cherry-picked merge commit 01b12dd with -x -m 1.
  • Resolved small context conflicts in RowCodec.cpp and gtest_table_info.cpp.

Checks

  • Not run, per request.

Release note

Fixed a TiFlash/TiKV mismatch after altering a column from NOT NULL to NULL

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Improved row decoding when encoded data is missing columns (especially during schema changes), with more reliable handling for NOT NULL and primary-key/handle-related columns.
    • Enhanced default/backfill behavior to better respect origin-default values and correct value construction for handle/primary-key columns.
  • Tests

    • Added unit tests for RegionBlockReader default handling and TiDB row codec missing-column scenarios (including origin-default parsing).
    • Added fullstack coverage to validate TiFlash/TiKV result consistency across NULLability transitions.

close pingcap#10680

ddl: Fix default value filling with finer granularity
- Tightens addDefaultValueToColumnIfPossible
  - For NOT NULL missing columns, force_decode=false now returns false unless there is an origin default.
  - This forces a schema sync instead of silently filling 0.
  - force_decode=true still fills a default value (best-effort).

Signed-off-by: JaySon-Huang <tshent@qq.com>
(cherry picked from commit 01b12dd)
@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 24, 2026
@coderabbitai

coderabbitai Bot commented Jun 24, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 4bfcbd07-19e1-4882-81df-44dd630a5335

📥 Commits

Reviewing files that changed from the base of the PR and between 589597a and 51dfa20.

📒 Files selected for processing (1)
  • dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp

📝 Walkthrough

Walkthrough

Adds origin-default detection, tightens missing-column decoding in RowCodec, and expands schema, region, and fullstack tests for NULLability and default-value cases.

Changes

NOT NULL column default-fill policy

Layer / File(s) Summary
ColumnInfo origin-default contract
dbms/src/TiDB/Schema/TiDB.h, dbms/src/TiDB/Schema/TiDB.cpp
Adds hasOriginDefaultValue() and documents origin-default conversion to Field.
RowCodec missing-column handling
dbms/src/TiDB/Decode/RowCodec.cpp
Updates default-fill decisions for missing columns, PK-flag handling, and V2 decode mismatch/remaining-column behavior.
Schema parsing tests
dbms/src/TiDB/tests/RowCodecTestUtils.h, dbms/src/TiDB/Schema/tests/gtest_table_info.cpp
Sets public schema state in test helpers and adds JSON parsing assertions for origin-default presence and values.
RegionBlockReader decoding paths
dbms/src/Storages/KVStore/Decode/RegionBlockReader.cpp, dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp
Clarifies read/decode comments, adjusts handle insertion for non-STRING PKs, and adds default-value region decoding coverage across five schema variants.
Fullstack NULLability regressions
tests/fullstack-test2/ddl/alter_column_nullable.test
Adds two TiFlash/TiKV comparison scenarios covering repeated column NULLability transitions.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • pingcap/tiflash#10682: Shares the same RowCodec missing-column handling and origin-default support.
  • pingcap/tiflash#10688: Touches the same default/origin-default decoding path in RowCodec.cpp and TiDB::ColumnInfo.
  • pingcap/tiflash#10689: Also updates addDefaultValueToColumnIfPossible and related force_decode behavior.

Suggested labels

type/cherry-pick-for-release-8.5

Suggested reviewers

  • JaySon-Huang
  • JinheLin
  • kolafish

Poem

🐇 A little block went missing in the snow,
origin_default helped it know where to go.
With force_decode set, the rows hopped in line,
And TiFlash and TiKV both read just fine.
🥕

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers the change, but it omits required template sections like issue number, problem summary, commit-message block, and checklist. Add the missing template sections: issue number/problem summary, a commit-message block, checklist items, and any side-effect or documentation notes.
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: fixing default value filling in DDL.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 24, 2026
@3pointer 3pointer marked this pull request as ready for review June 24, 2026 03:01
@ti-chi-bot ti-chi-bot Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2026
@ti-chi-bot ti-chi-bot Bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Jun 24, 2026
@ti-chi-bot ti-chi-bot Bot added the lgtm label Jun 24, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, JinheLin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [JaySon-Huang,JinheLin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot removed the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Jun 24, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

[LGTM Timeline notifier]

Timeline:

  • 2026-06-24 03:04:26.845858613 +0000 UTC m=+89550.509084126: ☑️ agreed by JaySon-Huang.
  • 2026-06-24 03:05:25.374559661 +0000 UTC m=+89609.037785174: ☑️ agreed by JinheLin.

@ti-chi-bot ti-chi-bot Bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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 `@dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp`:
- Around line 822-846: Fix the typos in the comment above the createColumn call
within the ASSERT_COLUMN_EQ macro. Change "thrid elem" to "third elem" and "wih"
to "with" in the comment that reads "the thrid elem is filled wih
origin_default".
- Around line 802-820: In the comment within the test block for
table_info_c1_not_null_with_origin_default, fix the spelling error where "thrid
elem" should be corrected to "third elem". This typo appears in the comment
above the ASSERT_COLUMN_EQ call that verifies the default value is filled
correctly.
🪄 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: bcd2c5a0-a404-4aa0-83f8-935e798a2e10

📥 Commits

Reviewing files that changed from the base of the PR and between e7d70a3 and c87716a.

📒 Files selected for processing (8)
  • dbms/src/Storages/KVStore/Decode/RegionBlockReader.cpp
  • dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp
  • dbms/src/TiDB/Decode/RowCodec.cpp
  • dbms/src/TiDB/Schema/TiDB.cpp
  • dbms/src/TiDB/Schema/TiDB.h
  • dbms/src/TiDB/Schema/tests/gtest_table_info.cpp
  • dbms/src/TiDB/tests/RowCodecTestUtils.h
  • tests/fullstack-test2/ddl/alter_column_nullable.test

Comment thread dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp
Comment thread dbms/src/Storages/KVStore/tests/gtest_region_block_reader.cpp
@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 24, 2026
@JaySon-Huang

Copy link
Copy Markdown
Contributor

/test pull-unit-test

@pingcap-cla-assistant

pingcap-cla-assistant Bot commented Jun 24, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@3pointer

Copy link
Copy Markdown
Author

/test pull-unit-test

@3pointer

Copy link
Copy Markdown
Author

/test pull-integration-test

@ti-chi-bot

ti-chi-bot Bot commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

@3pointer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-unit-test 51dfa20 link true /test pull-unit-test

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

@3pointer

Copy link
Copy Markdown
Author

/test pull-unit-test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants