-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
*: don't handle live updates of column size #58596
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #58596 +/- ##
================================================
+ Coverage 73.5326% 73.8917% +0.3590%
================================================
Files 1680 1710 +30
Lines 464728 474884 +10156
================================================
+ Hits 341727 350900 +9173
- Misses 102158 102404 +246
- Partials 20843 21580 +737
Flags with carried forward coverage won't be shown. Click here to find out more.
|
297fb21
to
111a58d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold
Thanks!
func (ctx *litTableMutateContext) UpdatePhysicalTableDelta( | ||
physicalTableID int64, _ int64, | ||
_ int64, cols variable.DeltaCols, | ||
func (*litTableMutateContext) UpdatePhysicalTableDelta( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we even delete this function? I believe we can delete the interface as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interface MutateContext
will be used in the normal write path. We cannot delete the MutateContext
interface.
We can only let the litTableMutateContext
not impl the interface StatisticsSupport
and return a nil
for its GetStatisticsSupport
and add an intest.Assert(false)
inside it.
@@ -257,13 +252,11 @@ type RemoveRecordOption interface { | |||
|
|||
// ExtraPartialRowOption is the combined one of IndexesLayout and ColumnSizeOption. | |||
type ExtraPartialRowOption struct { | |||
IndexesRowLayout IndexesLayout | |||
ColumnsSizeHelper *ColumnsSizeHelper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think struct ColumnsSizeHelper
can also be removed now.
I think the changes in the latest commit are incorrect. |
b63ca1d
to
1805653
Compare
some tests like |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
require.Equal(t, map[int64]int64{1: 2, 3: 9, 4: 3}, tblCtx.GetColumnSize(123)) | ||
// test gets a non-existed table | ||
require.Empty(t, tblCtx.GetColumnSize(456)) | ||
stats.UpdatePhysicalTableDelta(123, 5, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we can delete them at all.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lance6716, qw4990, Rustin170506 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What problem does this PR solve?
Issue Number: close #58595
Problem Summary:
What changed and how does it work?
Just as the issue said. We dropped the maintenance of the column size updates during various DML statements.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.