planner: use collected predicate columns to do stats sync load #56813
Conversation
ti-chi-bot
bot
added
release-note
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56813 +/- ##
================================================
+ Coverage 72.9101% 73.7650% +0.8548%
================================================
Files 1669 1699 +30
Lines 461114 469377 +8263
================================================
+ Hits 336199 346236 +10037
+ Misses 104270 101821 -2449
- Partials 20645 21320 +675
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ti-chi-bot
bot
added
approved
needs-1-more-lgtm
|
@winoros Please fix the linter first. |
winoros
force-pushed
the
simplify-stats-syncload
branch
from
fdebb75 to
cf41c5c
Compare
winoros
force-pushed
the
simplify-stats-syncload
branch
from
9009e6b to
8935218
Compare
| @@ -77,7 +77,7 @@ func TestPlanStatsLoad(t *testing.T) { | |||
| switch pp := p.(type) { | |||
| case *plannercore.PhysicalTableReader: | |||
| stats := pp.StatsInfo().HistColl | |||
| require.Equal(t, 0, countFullStats(stats, tableInfo.Columns[1].ID)) | |||
There was a problem hiding this comment.
The unneeded stats now is just fully unloaded.
| @@ -483,5 +483,6 @@ func TestPartialStatsInExplain(t *testing.T) { | |||
| output[i].Result = testdata.ConvertRowsToStrings(tk.MustQuery(sql).Rows()) | |||
| }) | |||
| tk.MustQuery(sql).Check(testkit.Rows(output[i].Result...)) | |||
| require.NoError(t, dom.StatsHandle().LoadNeededHistograms(dom.InfoSchema())) | |||
There was a problem hiding this comment.
Since only stats of predicates will be loaded, only stats of a from table tp is loaded into memory.
So for the first SQL, the stats of index ic and column b are not loaded into memory, becoming unInitialized. I use a LoadNeededHistograms to tell you that the actual stats is correct.
| if !fullLoad { | ||
| continue | ||
| } | ||
| if statsHandle == nil { |
There was a problem hiding this comment.
I guess we can move this check outside.
| } | ||
| tblInfo := tblID2TblInfo[neededCol.TableID] | ||
| if tblInfo == nil { | ||
| continue |
There was a problem hiding this comment.
Can you explain a little bit why this is possible?
| // If we already collected some columns that need trigger sync laoding on this table, we don't need to | ||
| // additionally do anything for determinate mode. | ||
| if physTblIDsWithNeededCols.Has(physicalTblID) || | ||
| statsHandle == nil { |
There was a problem hiding this comment.
If we do the stats handle check, I guess we don't need to do it here.
| } | ||
| // Choose the first column we meet to trigger stats loading. | ||
| if colToTriggerLoad == nil { | ||
| colToTriggerLoad = &model.TableItemID{TableID: int64(physicalTblID), ID: col.ID, IsIndex: false} |
There was a problem hiding this comment.
Do we need to break out here?
|
|
||
| // 2. get the stats table | ||
|
|
||
| // If we already collected some columns that need trigger sync laoding on this table, we don't need to |
There was a problem hiding this comment.
| // If we already collected some columns that need trigger sync laoding on this table, we don't need to | |
| // If we already collected some columns that need trigger sync loading on this table, we don't need to |
| // 2. get the stats table | ||
|
|
There was a problem hiding this comment.
| // 2. get the stats table | |
| // 2. get the stats table |
winoros
force-pushed
the
simplify-stats-syncload
branch
from
d589bd6 to
a602491
Compare
|
@winoros: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hawkingrei, Rustin170506 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 |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
/cherrypick release-8.5 |
|
@winoros: new pull request created to branch In response to this:
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 ti-community-infra/tichi repository. |
What problem does this PR solve?
Issue Number: close #56812
Problem Summary:
What changed and how does it work?
Just as the issue said, we can directly use predicate columns that we collect to decide the statistics' loading.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.