Skip to content

Storages: fix the range of load DMFilePackFilterResult#9755

Merged
ti-chi-bot[bot] merged 11 commits intopingcap:masterfrom
Lloyd-Pottiger:fix-rsfilter-range
Jan 3, 2025
Merged

Storages: fix the range of load DMFilePackFilterResult#9755
ti-chi-bot[bot] merged 11 commits intopingcap:masterfrom
Lloyd-Pottiger:fix-rsfilter-range

Conversation

@Lloyd-Pottiger
Copy link
Contributor

@Lloyd-Pottiger Lloyd-Pottiger commented Dec 31, 2024

What problem does this PR solve?

Issue Number: close #9754

Problem Summary:
Introduced by #9738

What is changed and how it works?


After #9738, we load the pack_filter_result in Segment::getInputStream. But the read_ranges is not shrunk by the segment_rowkey_range. So the pack_filter_result may contain pack does not related to the segment.

Shrink the read_ranges by segment_rowkey_range before generating pack_filter_result.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

None

Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
@ti-chi-bot ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 31, 2024
Signed-off-by: Lloyd-Pottiger <yan1579196623@gmail.com>
@JinheLin
Copy link
Contributor

JinheLin commented Jan 2, 2025

We should add more unit-tests to cover this issue.

Signed-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 2, 2025
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 2, 2025
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
Signed-off-by: JaySon-Huang <tshent@qq.com>
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 3, 2025
}
CATCH

TEST_P(DeltaMergeStoreRWTest, ReadAfterLogicalSplit)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test to cover this case.
Without the fix in Segment.cpp, this case will report error as below

/DATA/disk1/jaysonhuang/tiflash/dbms/src/Storages/DeltaMerge/tests/gtest_dm_delta_merge_store.cpp:590: Failure

  (in).read() return more rows(49152) than expected
  (createColumns({ createColumn<Int64>(createNumbers<Int64>(0, num_rows_write)), })).rows()
    Which is: 24576

@JaySon-Huang
Copy link
Contributor

/cc @CalvinNeo @JinheLin PTAL

@ti-chi-bot ti-chi-bot bot requested review from CalvinNeo and JinheLin January 3, 2025 03:53
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 3, 2025

@JaySon-Huang: GitHub didn't allow me to request PR reviews from the following users: PTAL.

Note that only pingcap members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @CalvinNeo @JinheLin PTAL

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.

Copy link
Member

@CalvinNeo CalvinNeo left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 3, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CalvinNeo, 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:

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 added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Jan 3, 2025
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jan 3, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-01-03 06:15:29.4943808 +0000 UTC m=+676664.850385339: ☑️ agreed by CalvinNeo.
  • 2025-01-03 06:17:50.640736096 +0000 UTC m=+676805.996740635: ☑️ agreed by JinheLin.

@JaySon-Huang
Copy link
Contributor

/test pull-unit-test

@ti-chi-bot ti-chi-bot bot merged commit 4ed360a into pingcap:master Jan 3, 2025
@Lloyd-Pottiger Lloyd-Pottiger deleted the fix-rsfilter-range branch January 6, 2025 01:50
gengliqi added a commit to gengliqi/tiflash that referenced this pull request Jan 21, 2025
yongman pushed a commit to yongman/tiflash that referenced this pull request Jun 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm release-note-none Denotes a PR that doesn't merit a release note. 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.

Query result inconsistent

4 participants