planner: fix that vector index output empty result when pk is non-int type #57629

winoros · 2024-11-22T07:29:34Z

What problem does this PR solve?

Issue Number: close #57627

Problem Summary:

What changed and how does it work?

When pk is clustered and non-int, the full range should be [MinNotNull, MaxValue]. But it's [MinInt64, MaxInt64] currently. So it will output an empty result.

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

Release note

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

None

… type

codecov · 2024-11-22T07:48:51Z

Codecov Report

Attention: Patch coverage is 82.35294% with 3 lines in your changes missing coverage. Please review.

Project coverage is 73.5972%. Comparing base (3016889) to head (d08472b).
Report is 5 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #57629        +/-   ##
================================================
+ Coverage   72.7988%   73.5972%   +0.7984%     
================================================
  Files          1677       1677                
  Lines        463954     465868      +1914     
================================================
+ Hits         337753     342866      +5113     
+ Misses       105320     102272      -3048     
+ Partials      20881      20730       -151

Flag	Coverage Δ
integration	`43.7974% <11.7647%> (?)`
unit	`72.4401% <82.3529%> (+0.2484%)`	⬆️

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

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`46.2894% <ø> (+0.7511%)`	⬆️

winoros · 2024-11-22T08:34:45Z

pkg/planner/core/find_best_task.go

+	if prop.VectorProp.VectorHelper != nil && path.Index != nil && path.Index.VectorInfo != nil {
+		if path.Index == nil || path.Index.VectorInfo == nil {
+			return false
+		}
+		if ds.TableInfo.Columns[path.Index.Columns[0].Offset].ID != prop.VectorProp.Column.ID {
+			return false
+		}
+
+		if model.IndexableFnNameToDistanceMetric[prop.VectorProp.DistanceFnName.L] != path.Index.VectorInfo.DistanceMetric {
+			return false
+		}
+		return true
+	}


I changed it to the highest priority.
Because when for the vector index path, the IsIntHandlePath can also be true. We need to use a new way to check the real pk path.

winoros · 2024-11-22T09:13:51Z

/retest

winoros · 2024-11-22T09:29:09Z

/retest

zimulala · 2024-11-25T03:55:06Z

pkg/planner/core/planbuilder.go

 			if index.VectorInfo != nil {
 				// Because the value of `TiFlashReplica.Available` changes as the user modify replica, it is not ideal if the state of index changes accordingly.
 				// So the current way to use the vector indexes is to require the TiFlash Replica to be available.
 				if !tblInfo.TiFlashReplica.Available {
 					continue
 				}
+				path := genTiFlashPath(tblInfo)


I feel that the original logic is different here set up "tablePath. IsCommonHandlePath = true". What does this setting do? Why is the original logic not needed？

A normal index will create the index's ranges.
But the vector index still uses the table range. So we need to correctly set the pk type. So it can generate the correct table range.
For int pk, it will use [MinInt64, MaxInt64] as full range. But for other cases, [MinNotNull, MaxValue] will be full range.

i think if we don't set it here, we will step into here right? which will derive a int range for common handle tiflash path

…ingcap#57430) close pingcap#53490

…irectly displayed by default behavior (pingcap#57626) close pingcap#57625

hawkingrei · 2024-11-26T03:31:15Z

/retest

hawkingrei · 2024-11-26T03:43:34Z

/test fast_test_tiprow

hawkingrei · 2024-11-26T03:49:17Z

/test fast_test_tiprow

tiprow · 2024-11-26T03:49:34Z

@ti-chi-bot[bot]: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test fast_test_tiprow
/test fast_test_tiprow_ddlargsv1
/test tidb_parser_test

Use /test all to run the following jobs that were automatically triggered:

fast_test_tiprow
tidb_parser_test

In response to this:

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:
/test build
/test check-dev
/test check-dev2
/test mysql-test
/test pull-br-integration-test
/test pull-integration-ddl-test
/test pull-integration-e2e-test
/test pull-lightning-integration-test
/test pull-mysql-client-test
/test pull-unit-test-ddlv1
/test unit-test
The following commands are available to trigger optional jobs:
/test pingcap/tidb/canary_ghpr_unit_test
/test pull-common-test
/test pull-e2e-test
/test pull-integration-common-test
/test pull-integration-copr-test
/test pull-integration-jdbc-test
/test pull-integration-mysql-test
/test pull-integration-nodejs-test
/test pull-integration-python-orm-test
/test pull-sqllogic-test
/test pull-tiflash-test
Use /test all to run the following jobs that were automatically triggered:
pingcap/tidb/ghpr_build
pingcap/tidb/ghpr_check
pingcap/tidb/ghpr_check2
pingcap/tidb/ghpr_mysql_test
pingcap/tidb/ghpr_unit_test
pingcap/tidb/pull_integration_ddl_test
pingcap/tidb/pull_integration_e2e_test
pingcap/tidb/pull_mysql_client_test
In response to this:

/test fast_test_tiprow

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.

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.

hawkingrei · 2024-11-26T10:04:01Z

/retest

hawkingrei · 2024-11-26T10:04:18Z

/test all

winoros · 2024-11-26T11:14:07Z

/retest

hawkingrei · 2024-11-26T15:00:28Z

/test all

AilinKid · 2024-11-26T15:58:43Z

pkg/planner/core/logical_plan_builder.go

+		// Avoid adding CTE table to the SELECT privilege list, maybe we have better way to do this?
+		if _, ok := b.nameMapCTE[t.Name.L]; !ok {
+			b.visitInfo = appendVisitInfo(b.visitInfo, mysql.SelectPriv, dbName, t.Name.L, "", nil)
+		}


why should we need this for? any direct effect from the issue?

tiprow · 2024-11-27T00:27:26Z

@winoros: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
fast_test_tiprow	`d08472b`	link	true	`/test fast_test_tiprow`

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.

ti-chi-bot · 2024-11-27T00:56:44Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~pkg/planner/OWNERS~~ [AilinKid]

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 · 2024-11-27T00:56:48Z

[LGTM Timeline notifier]

Timeline:

2024-11-26 16:05:59.323865406 +0000 UTC m=+566146.943519908: ☑️ agreed by AilinKid.
2024-11-27 00:56:47.014437095 +0000 UTC m=+597994.634091609: ☑️ agreed by breezewish.

ti-chi-bot · 2024-11-27T01:39:11Z

In response to a cherrypick label: new pull request created to branch release-8.5: #57744.

… type (#57629) (#57744) close #57627

planner: fix that vector index output empty result when pk is non-int…

38e74df

… type

ti-chi-bot bot added do-not-merge/needs-tests-checked release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2024

winoros changed the title ~~planner: fix that vector index output empty result when pk is non-int…~~ planner: fix that vector index output empty result when pk is non-int type Nov 22, 2024

fix tests

6c41ea2

ti-chi-bot bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 22, 2024

winoros commented Nov 22, 2024

View reviewed changes

ti-chi-bot bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Nov 22, 2024

zimulala reviewed Nov 25, 2024

View reviewed changes

bb7133 and others added 2 commits November 26, 2024 11:10

planner/core: fix a wrong privilege check for CTE & UPDATE statement (p…

ec3fb7b

…ingcap#57430) close pingcap#53490

planner, runtime_filter: Remove redundant logs whose meaning can be d…

2ba6fda

…irectly displayed by default behavior (pingcap#57626) close pingcap#57625

ti-chi-bot bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-tests-checked labels Nov 26, 2024

add unit test

eca8aca

winoros force-pushed the vectorindex-nonintpk branch from dead388 to eca8aca Compare November 26, 2024 03:23

pingcap deleted a comment from ti-chi-bot bot Nov 26, 2024

pingcap deleted a comment from tiprow bot Nov 26, 2024

pingcap deleted a comment from ti-chi-bot bot Nov 26, 2024

fix static check

ccb08e7

winoros force-pushed the vectorindex-nonintpk branch from ccdd1e0 to ccb08e7 Compare November 26, 2024 06:07

AilinKid reviewed Nov 26, 2024

View reviewed changes

Merge remote-tracking branch 'origin/master' into vectorindex-nonintpk

d08472b

AilinKid approved these changes Nov 26, 2024

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Nov 26, 2024

breezewish approved these changes Nov 27, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 27, 2024

ti-chi-bot bot merged commit feb34ec into pingcap:master Nov 27, 2024
23 of 24 checks passed

ti-chi-bot mentioned this pull request Nov 27, 2024

planner: fix that vector index output empty result when pk is non-int type (#57629) #57744

Merged

13 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Nov 27, 2024

planner: fix that vector index output empty result when pk is non-int…

f89b6ab

… type (#57629) (#57744) close #57627

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

planner: fix that vector index output empty result when pk is non-int type #57629

planner: fix that vector index output empty result when pk is non-int type #57629

winoros commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

winoros Nov 22, 2024

winoros commented Nov 22, 2024

winoros commented Nov 22, 2024

zimulala Nov 25, 2024

winoros Nov 26, 2024

AilinKid Nov 26, 2024 •

edited

Loading

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

winoros commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

AilinKid Nov 26, 2024

tiprow bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot commented Nov 27, 2024

planner: fix that vector index output empty result when pk is non-int type #57629

planner: fix that vector index output empty result when pk is non-int type #57629

Conversation

winoros commented Nov 22, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

winoros Nov 22, 2024

Choose a reason for hiding this comment

winoros commented Nov 22, 2024

winoros commented Nov 22, 2024

zimulala Nov 25, 2024

Choose a reason for hiding this comment

winoros Nov 26, 2024

Choose a reason for hiding this comment

AilinKid Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

tiprow bot commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

winoros commented Nov 26, 2024

hawkingrei commented Nov 26, 2024

AilinKid Nov 26, 2024

Choose a reason for hiding this comment

tiprow bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

ti-chi-bot bot commented Nov 27, 2024

[LGTM Timeline notifier]

ti-chi-bot commented Nov 27, 2024

winoros commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

AilinKid Nov 26, 2024 •

edited

Loading