planner: speed up PhysicalTopN#containVirtualColumn #46812

tedyu · 2023-09-09T14:05:48Z

What problem does this PR solve?

Issue Number: close #46809

Problem Summary:
Using nested loop is inefficient when column count is high.

What is changed and how it works?

This is a refactoring of PhysicalTopN#containVirtualColumn.
At the beginning, we establish a map from column Id to *expression.Column.
In the main loop, we just check whether the underlying column Id is in the map.

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

Release note

None

ti-chi-bot · 2023-09-09T14:05:58Z

Hi @tedyu. Thanks for your PR.

I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

tiprow · 2023-09-09T14:06:13Z

Hi @tedyu. 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/test-infra repository.

tedyu · 2023-09-09T15:24:07Z

cc @hawkingrei

hawkingrei · 2023-09-09T15:39:40Z

/ok-to-test

planner/core/task.go

tedyu · 2023-09-09T15:47:57Z

make[1]: Leaving directory `/home/jenkins/agent/workspace/pingcap/tidb/ghpr_unit_test/tidb'
make: *** [check-bazel-prepare] Error 2
root: /home/jenkins/.tidb/tmp/63a9840cd0739f2c243bb46478607469/execroot/__main__/bazel-out/k8-fastbuild/testlogs
mv: cannot stat 'bazel.xml': No such file or directory
script returned exit code 2

It seems the above error was not related to my PR.

codecov · 2023-09-09T16:01:48Z

Codecov Report

Merging #46812 (9d70107) into master (d3d30f5) will decrease coverage by 0.6261%.
The diff coverage is 100.0000%.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #46812        +/-   ##
================================================
- Coverage   73.3326%   72.7066%   -0.6261%     
================================================
  Files          1322       1343        +21     
  Lines        396474     402761      +6287     
================================================
+ Hits         290745     292834      +2089     
- Misses        87208      91363      +4155     
- Partials      18521      18564        +43

Flag	Coverage Δ
integration	`27.8648% <71.4285%> (?)`
unit	`73.3236% <100.0000%> (-0.0091%)`	⬇️

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

Components	Coverage Δ
dumpling	`54.0444% <ø> (ø)`
parser	`84.9609% <ø> (-0.0108%)`	⬇️
br	`48.2837% <ø> (-4.3580%)`	⬇️

hawkingrei · 2023-09-09T16:15:48Z

make[1]: Leaving directory `/home/jenkins/agent/workspace/pingcap/tidb/ghpr_unit_test/tidb'
make: *** [check-bazel-prepare] Error 2
root: /home/jenkins/.tidb/tmp/63a9840cd0739f2c243bb46478607469/execroot/__main__/bazel-out/k8-fastbuild/testlogs
mv: cannot stat 'bazel.xml': No such file or directory
script returned exit code 2

It seems the above error was not related to my PR.

Yes, I think it is about our CI.

tedyu · 2023-09-09T16:41:53Z

 [ERROR] [runaway.go:145] ["try to get new runaway watch"] [error="[schema:1146]Table 'mysql.tidb_runaway_watch' doesn't exist"]

The above error doesn't seem to be related to my PR.

I wonder if this may happen before all the upgrade scripts in bootstrapVersion complete execution. In this case, upgradeToVer172 may not have run.

tedyu · 2023-09-09T18:29:36Z

/test all

tedyu · 2023-09-09T21:34:50Z

/test all

hawkingrei · 2023-09-10T01:32:55Z

 [ERROR] [runaway.go:145] ["try to get new runaway watch"] [error="[schema:1146]Table 'mysql.tidb_runaway_watch' doesn't exist"]
The above error doesn't seem to be related to my PR.

I wonder if this may happen before all the upgrade scripts in bootstrapVersion complete execution. In this case, upgradeToVer172 may not have run.

I found it. it is not your problem.

hawkingrei · 2023-09-10T01:33:27Z

/test all

hawkingrei · 2023-09-10T02:29:35Z

/test unit-test

tiprow · 2023-09-10T02:29:47Z

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test unit-test

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/test-infra repository.

hawkingrei · 2023-09-10T02:29:54Z

/test check-dev

tiprow · 2023-09-10T02:30:09Z

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test tiprow_fast_test

Use /test all to run all jobs.

In response to this:

/test check-dev

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/test-infra repository.

ti-chi-bot · 2023-09-11T02:47:35Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~OWNERS~~ [hawkingrei,qw4990]

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 · 2023-09-11T02:47:39Z

[LGTM Timeline notifier]

Timeline:

2023-09-09 15:39:58.977356923 +0000 UTC m=+110980.901913317: ☑️ agreed by hawkingrei.
2023-09-11 02:47:37.993231868 +0000 UTC m=+237439.917788262: ☑️ agreed by qw4990.

Rustin170506

👍🏿

If we can add a golang benchmark to it, that would be nice.

Rustin170506 · 2023-09-11T02:47:31Z

planner/core/task.go

 	for _, by := range p.ByItems {
 		cols := expression.ExtractColumns(by.Expr)
 		for _, col := range cols {
-			for _, tCol := range tCols {
+			if _, ok := tColSet[col.ID]; ok {
 				// A column with ID > 0 indicates that the column can be resolved by data source.


Could you please move this comment to the right place? Thanks!

the PR has been merged.
If I happen to modify this class again, I can move it.

I think it is fine to leave the comment here as well.

tedyu force-pushed the cont-virt-col branch from a276c4f to aae5b8d Compare September 9, 2023 15:09

ti-chi-bot bot added ok-to-test Indicates a PR is ready to be tested. and removed needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Sep 9, 2023

hawkingrei approved these changes Sep 9, 2023

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 9, 2023

hawkingrei reviewed Sep 9, 2023

View reviewed changes

planner/core/task.go Outdated Show resolved Hide resolved

tedyu force-pushed the cont-virt-col branch from aae5b8d to 3c95e1c Compare September 9, 2023 15:49

planner: speed up PhysicalTopN#containVirtualColumn

9d70107

tedyu force-pushed the cont-virt-col branch from 3c95e1c to 9d70107 Compare September 9, 2023 16:01

hawkingrei requested review from xhebox and winoros September 10, 2023 06:07

qw4990 approved these changes Sep 11, 2023

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 11, 2023

ti-chi-bot bot merged commit 262327f into pingcap:master Sep 11, 2023

Rustin170506 reviewed Sep 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: speed up PhysicalTopN#containVirtualColumn #46812

planner: speed up PhysicalTopN#containVirtualColumn #46812

tedyu commented Sep 9, 2023 •

edited

Loading

ti-chi-bot bot commented Sep 9, 2023

tiprow bot commented Sep 9, 2023

tedyu commented Sep 9, 2023

hawkingrei commented Sep 9, 2023

tedyu commented Sep 9, 2023

codecov bot commented Sep 9, 2023 •

edited

Loading

hawkingrei commented Sep 9, 2023

tedyu commented Sep 9, 2023 •

edited

Loading

tedyu commented Sep 9, 2023

tedyu commented Sep 9, 2023

hawkingrei commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

tiprow bot commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

tiprow bot commented Sep 10, 2023

ti-chi-bot bot commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

Rustin170506 left a comment

Rustin170506 Sep 11, 2023

tedyu Sep 11, 2023

planner: speed up PhysicalTopN#containVirtualColumn #46812

planner: speed up PhysicalTopN#containVirtualColumn #46812

Conversation

tedyu commented Sep 9, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot bot commented Sep 9, 2023

tiprow bot commented Sep 9, 2023

tedyu commented Sep 9, 2023

hawkingrei commented Sep 9, 2023

tedyu commented Sep 9, 2023

codecov bot commented Sep 9, 2023 • edited Loading

Codecov Report

hawkingrei commented Sep 9, 2023

tedyu commented Sep 9, 2023 • edited Loading

tedyu commented Sep 9, 2023

tedyu commented Sep 9, 2023

hawkingrei commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

tiprow bot commented Sep 10, 2023

hawkingrei commented Sep 10, 2023

tiprow bot commented Sep 10, 2023

ti-chi-bot bot commented Sep 11, 2023

ti-chi-bot bot commented Sep 11, 2023

[LGTM Timeline notifier]

Rustin170506 left a comment

Choose a reason for hiding this comment

Rustin170506 Sep 11, 2023

Choose a reason for hiding this comment

tedyu Sep 11, 2023

Choose a reason for hiding this comment

tedyu commented Sep 9, 2023 •

edited

Loading

codecov bot commented Sep 9, 2023 •

edited

Loading

tedyu commented Sep 9, 2023 •

edited

Loading