Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

planner: code prepare for support OR list nested in AND list for mv index #51780

Merged
merged 8 commits into from Mar 14, 2024

Conversation

time-and-fate
Copy link
Member

@time-and-fate time-and-fate commented Mar 14, 2024

What problem does this PR solve?

Issue Number: ref #51778

Problem Summary:

Some code preparation to make #51716 smaller.

What changed and how does it work?

  • Add test cases for comparison in the next PR. (tests/integrationtest/r/planner/core/indexmerge_path.result, tests/integrationtest/t/planner/core/indexmerge_path.test)
  • Fix two test cases where the values in the SQL text use the wrong type. (pkg/planner/core/indexmerge_path_test.go)
  • Change the passed parameters in several functions. The effect of the functions are not affected. (pkg/planner/core/indexmerge_path.go)
  • Enhance several existing methods. (pkg/planner/core/indexmerge_path.go)
    • PrepareCols4MVIndex() -> PrepareIdxColsAndUncoverArrayType(): Add a parameter to allow this function not to check there is exactly 1 virtual column. In the next PR, we'll use this function for the non-MV index.
    • jsonArrayExpr2Exprs(): Add a parameter to allow skipping the check for skipping plan cache. This will be used if we are not building an access path and are only doing checks and collecting information. This will be used in the next function.
    • checkFilter4MVIndexColumn()
      • Before, we only do simple checks on the expression to see if it can be an access filter. Now, we'll also extract values from the expression and do similar checks as in buildPartialPaths4MVIndex(). Using these values, we also return the "type" of the access filter.
    • collectFilters4MVIndex(): Based on changes in checkFilter4MVIndexColumn(), provide a "type" for the access filters in the return value.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No need to test
    • I checked and no code files have been changed.

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

Please refer to Release Notes Language Style Guide to write a quality release note.

None

@ti-chi-bot ti-chi-bot bot added release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 14, 2024
Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Merging #51780 (4a3276f) into master (0ed511a) will decrease coverage by 3.7537%.
The diff coverage is 91.8750%.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #51780        +/-   ##
================================================
- Coverage   72.5274%   68.7737%   -3.7537%     
================================================
  Files          1476       1476                
  Lines        364152     433645     +69493     
================================================
+ Hits         264110     298234     +34124     
- Misses        80658     115396     +34738     
- Partials      19384      20015       +631     
Flag Coverage Δ
integration 48.6538% <91.8750%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 56.2971% <ø> (ø)
parser ∅ <ø> (∅)
br 48.2196% <ø> (+1.7314%) ⬆️

eqOnNonMVColTp
multiValuesOROnMVColTp
multiValuesANDOnMVColTp
singleValueOnMVColTp
Copy link
Contributor

Choose a reason for hiding this comment

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

you can add more comments on the situation they stands

pkg/planner/core/indexmerge_path.go Show resolved Hide resolved
pkg/planner/core/indexmerge_path.go Show resolved Hide resolved
Copy link

ti-chi-bot bot commented Mar 14, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AilinKid, qw4990

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

The pull request process is described here

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

Copy link

ti-chi-bot bot commented Mar 14, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-03-14 09:34:29.644359888 +0000 UTC m=+937296.666606277: ☑️ agreed by qw4990.
  • 2024-03-14 16:30:12.674902196 +0000 UTC m=+962239.697148585: ☑️ agreed by AilinKid.

@ti-chi-bot ti-chi-bot bot merged commit 6a76187 into pingcap:master Mar 14, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm release-note-none size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants