Skip to content

Conversation

kirillvasilenko
Copy link
Collaborator

@kirillvasilenko kirillvasilenko commented Oct 14, 2025

fixes #26726

Here we reuse the existing way of understanding if a portion is conflicting with the current tx, and build the stable meaningful state that the scan logic uses.

Also noticed that TSourceConstructor.SourceId field is ui32, but we pass there ui64 value (portionId). So I fixed that as well.

Copy link

github-actions bot commented Oct 14, 2025

🟢 2025-10-14 18:11:42 UTC The validation of the Pull Request description is successful.

Copy link

github-actions bot commented Oct 14, 2025

2025-10-14 17:38:12 UTC Pre-commit check linux-x86_64-relwithdebinfo for dc02b8d has started.
2025-10-14 17:38:25 UTC Artifacts will be uploaded here
2025-10-14 17:41:59 UTC ya make is running...
2025-10-14 18:07:09 UTC Check cancelled

Copy link

github-actions bot commented Oct 14, 2025

2025-10-14 17:38:22 UTC Pre-commit check linux-x86_64-release-asan for dc02b8d has started.
2025-10-14 17:38:35 UTC Artifacts will be uploaded here
2025-10-14 17:42:10 UTC ya make is running...
2025-10-14 18:07:11 UTC Check cancelled

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a mechanism to provide a stable view of portion states during the entire scan operation in Column Shards. The changes replace the previous EPortionCommitStatus enum-based approach with a new TPortionStateAtScanStart struct that captures portion state at scan initiation time, preventing anomalies caused by portions changing state during scan execution.

  • Replaced EPortionCommitStatus enum with TPortionStateAtScanStart struct containing committed, conflicting, and snapshot information
  • Updated logic in both simple and plain readers to use the new stable state mechanism
  • Refactored conflict detection and duplicate filtering to work with the new state representation

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
ydb/core/tx/columnshard/engines/reader/simple_reader/iterator/source.cpp Updated portion state checking and conflict detection logic
ydb/core/tx/columnshard/engines/reader/simple_reader/iterator/context.cpp Modified fetching plan logic and duplicate filtering to use new state
ydb/core/tx/columnshard/engines/reader/simple_reader/iterator/collections/constructors.h Changed SourceId type and reformatted constructor
ydb/core/tx/columnshard/engines/reader/plain_reader/iterator/source.cpp Updated portion state checking and conflict detection logic
ydb/core/tx/columnshard/engines/reader/common_reader/iterator/context.h Replaced enum with new struct and added state determination logic
ydb/core/tx/columnshard/engines/reader/common_reader/iterator/context.cpp Implemented comprehensive portion state determination logic
ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h Added new method and removed old one
ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.cpp Updated initialization logic and removed old method
ydb/core/tx/columnshard/engines/portions/written.h Reorganized method placement in class

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

github-actions bot commented Oct 14, 2025

2025-10-14 18:10:15 UTC Pre-commit check linux-x86_64-release-asan for 6c97773 has started.
2025-10-14 18:10:29 UTC Artifacts will be uploaded here
2025-10-14 18:14:15 UTC ya make is running...
🟡 2025-10-14 20:32:14 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
15862 15408 0 142 292 20

🟢 2025-10-14 20:32:19 UTC Build successful.
🟢 2025-10-14 20:32:42 UTC ydbd size 3.8 GiB changed* by -56 Bytes, which is <= 0 Bytes vs main: OK

ydbd size dash main: 1673452 merge: 6c97773 diff diff %
ydbd size 4 031 887 144 Bytes 4 031 887 088 Bytes -56 Bytes -0.000%
ydbd stripped size 1 497 433 600 Bytes 1 497 433 696 Bytes +96 Bytes +0.000%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

Copy link

github-actions bot commented Oct 14, 2025

2025-10-14 18:10:51 UTC Pre-commit check linux-x86_64-relwithdebinfo for 6c97773 has started.
2025-10-14 18:11:05 UTC Artifacts will be uploaded here
2025-10-14 18:14:41 UTC ya make is running...
🟡 2025-10-14 19:53:06 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
39025 36254 0 1 2744 26

2025-10-14 19:53:14 UTC ya make is running... (failed tests rerun, try 2)
🟢 2025-10-14 20:10:49 UTC Tests successful.

Ya make output | Test bloat | Test bloat

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
730 (only retried tests) 708 0 0 0 22

🟢 2025-10-14 20:10:54 UTC Build successful.
🟢 2025-10-14 20:11:12 UTC ydbd size 2.3 GiB changed* by +43.5 KiB, which is < 100.0 KiB vs main: OK

ydbd size dash main: 9c08324 merge: 6c97773 diff diff %
ydbd size 2 423 377 720 Bytes 2 423 422 272 Bytes +43.5 KiB +0.002%
ydbd stripped size 515 870 856 Bytes 515 874 056 Bytes +3.1 KiB +0.001%

*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation

@kirillvasilenko kirillvasilenko marked this pull request as ready for review October 15, 2025 08:21
@kirillvasilenko kirillvasilenko requested a review from a team as a code owner October 15, 2025 08:21
private:
TCompareKeyForScanSequence Start;
YDB_READONLY(ui32, SourceId, 0);
YDB_READONLY(ui64, SourceId, 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

хм, а portion id получается ui64?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Получается так. Прямо в этом классе есть метод ui64 DoGetEntityId() который возвращает SourceId. Плюс конструктор выставляет это поле вот так SourceId(portion->GetPortionId()), portion->GetPortionId() тоже возвращает ui64.

@kirillvasilenko kirillvasilenko merged commit afb0172 into ydb-platform:main Oct 17, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Let the scan see stable state of portions during the whole scan in Column Shards

2 participants