planner/core: update missing virtual columns in update and insert

[joechenrh]

What problem does this PR solve?

Issue Number: close #58400

Problem Summary:

In #58494, we fixed a problem related to virtual columns. But we haven't completely fixed it yet. There still have problems with virtual columns.

For Update:

In update statements, virtual columns will be added into UpdateLists if their dependent columns have changed. #55829 has fixed nested generated column.

tidb/pkg/planner/core/logical_plan_builder.go

Lines 5826 to 5837 in f2db9c4

	// check if the column is modified
	dependentColumns := expression.ExtractDependentColumns(newExpr)
	var isModified bool
	for _, col := range dependentColumns {
	if dependentColumnsModified[col.UniqueID] {
	isModified = true
	break
	}
	}
	if isModified {
	dependentColumnsModified[col.UniqueID] = true
	}

However, we still ignore the columns with OnUpdateNow flag when checking dependency. Since these columns may be updated in the execution stage too.

For Insert:

For SQL like INSERT INTO ... ON DUPLICATE KEY UPDATE ..., only virtual columns that directly depend on the assignments after UPDATE will be updated. Take the table in #58400 as example:

CREATE TABLE tmv (
  J1 json, J2 json GENERATED ALWAYS AS (j1) VIRTUAL,
  UNIQUE KEY i1 ((cast(j1 as signed array))),
  KEY i2 ((cast(j2 as signed array)))
);

INSERT INTO tmv SET j1 = '[1]' ON DUPLICATE KEY UPDATE j1 = '[2]';

The dependencies of each column are listed below. Only j2 and _V$_i1_0 are updated since they depends on j1. We should handle it the same way as what we do in #55829.

j2 <-- j1
_V$_i1_0 <-- j1
_V$_i2_0 <-- j2

What changed and how does it work?

In summary, when extracting generated columns, we have to consider both chain dependencies and on-update-now flag too.

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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign rustin170506 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS
• pkg/planner/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tiprow]

Hi @joechenrh. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.2491%. Comparing base (a3574aa) to head (445b914).
Report is 39 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #58401        +/-   ##
================================================
- Coverage   73.5552%   73.2491%   -0.3061%     
================================================
  Files          1680       1681         +1     
  Lines        463684     472266      +8582     
================================================
+ Hits         341064     345931      +4867     
- Misses       101817     105463      +3646     
- Partials      20803      20872        +69     

Flag	Coverage Δ
integration	43.1508% <100.0000%> (?)
unit	72.3696% <100.0000%> (+0.0622%)	⬆️

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

Components	Coverage Δ
dumpling	52.6910% <ø> (ø)
parser	∅ <ø> (∅)
br	44.0985% <ø> (-1.8825%)	⬇️

[ti-chi-bot]

@joechenrh: once the present PR merges, I will cherry-pick it on top of release-7.1 in the new PR and assign it to you.

In response to this:

/cherry-pick release-7.1

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.

[AilinKid]

Have we set the order of columns following the asc dependency?
for _, column := range columns
cuz we only set the onDups for the later coming columns, those prev ones couldn't see it any more.

[joechenrh]

cuz we only set the onDups for the later coming columns, those prev ones couldn't see it any more.

Yes, you are right. After testing, I found that this situation may be possible to happen.

For CREATE TABLE, a generated column can't depend on columns after it.

But for ALTER TABLE ADD COLUMN, we can use AFTER keyword to force the new generated column inserted before its dependent column and get a table like:

mysql> show create table t1;
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table                                                                                                                                                                              |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| t1    | CREATE TABLE `t1` (
  `id` int DEFAULT NULL,
  `col2` int GENERATED ALWAYS AS (`col1` + 1) VIRTUAL,
  `col1` int DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin |
+-------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Let me think about how to modify it.

[joechenrh]

There are something wrong in the previous comment.

In resolveGeneratedColumns, we only process generated columns. Non-generated columns are already processed and stored in onDups:

tidb/pkg/planner/core/planbuilder.go

Line 3859 in 43f2fb9

func (b *PlanBuilder) resolveGeneratedColumns(ctx context.Context, columns []*table.Column, onDups map[string]struct{}, mockPlan base.LogicalPlan) (igc InsertGeneratedColumns, err error) {

Since generated columns can only depends on columns prior to it, we don't need to handle the order of columns.

mysql> alter table t1 add column col3 int generated always as (col2 + 1) virtual after id;                                                                                                    
ERROR 3107 (HY000): Generated column can refer only to generated columns defined prior to it.