Skip to content

Conversation

L-maple
Copy link
Contributor

@L-maple L-maple commented Jan 23, 2025

…ant of different types

What problem does this PR solve?

Issue Number: close #59123

Problem Summary:

What changed and how does it work?

SQL is a weak type language, and users always ignore constant types when comparing partition key.
When try to prune partitions, try more types of constant. For now, we should consider real and decimal, which are regularly used.

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 ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-triage-completed sig/planner SIG: Planner needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 23, 2025
Copy link

ti-chi-bot bot commented Jan 23, 2025

Hi @L-maple. 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-sigs/prow repository.

Copy link

tiprow bot commented Jan 23, 2025

Hi @L-maple. 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.

@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from 7a2d704 to 5abd231 Compare January 23, 2025 07:50
@L-maple
Copy link
Contributor Author

L-maple commented Jan 23, 2025

@Defined2014 Hi,I find a problem incompatible with MySQL. Could you help me review the PR please?

@L-maple
Copy link
Contributor Author

L-maple commented Jan 23, 2025

/assign @L-maple

@Defined2014 Defined2014 changed the title core: enhance partition prune when comparing partition key with constant of different types planner: enhance partition prune when comparing partition key with constant of different types Jan 23, 2025
@Defined2014 Defined2014 requested a review from mjonss January 23, 2025 10:04
@Defined2014 Defined2014 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 Jan 23, 2025
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work also with RANGE COLUMNS partitioning and non numeric data types?

@wuhuizuo
Copy link
Contributor

wuhuizuo commented Feb 6, 2025

/retest

a new trigger command to refresh issue triage status.

@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from 5abd231 to b923f16 Compare February 6, 2025 09:48
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the integration tests and fix this one:

% cd tests/integrationtest
% ./run-tests.sh -s ../../bin/tidb-server -t planner/core/casetest/partition/partition_pruner

You can probably just record the new output with:

./run-tests.sh -s ../../bin/tidb-server -r planner/core/casetest/partition/partition_pruner

@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from b923f16 to 002fec5 Compare February 7, 2025 08:55
@ti-chi-bot ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 7, 2025
@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from 002fec5 to 60fc457 Compare February 7, 2025 10:03

// case 4: range columns partition
tk.MustExec(`CREATE TABLE t_range_col(a int, b int)
PARTITION BY RANGE COLUMNS(a) (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a different datatype if testing COLUMNS partitioning, since that is the main usage (apart from partitioning on multiple columns, which is not as useful).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will complete the task,thanks for your advice!

Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution and good work!

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Feb 7, 2025
@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch 2 times, most recently from edcd8bd to 7e4a001 Compare February 7, 2025 12:25
Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 74.62687% with 17 lines in your changes missing coverage. Please review.

Project coverage is 73.4783%. Comparing base (c60c841) to head (48f5dfe).
Report is 31 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #59155        +/-   ##
================================================
+ Coverage   73.0975%   73.4783%   +0.3807%     
================================================
  Files          1689       1690         +1     
  Lines        467025     471169      +4144     
================================================
+ Hits         341384     346207      +4823     
+ Misses       104677     103867       -810     
- Partials      20964      21095       +131     
Flag Coverage Δ
integration 43.0333% <59.7014%> (?)
unit 72.2519% <74.6268%> (-0.0370%) ⬇️

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

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

@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from 7e4a001 to e58964b Compare February 8, 2025 06:51
@L-maple
Copy link
Contributor Author

L-maple commented Feb 10, 2025

@mjonss @Defined2014 It seems fast_test_tiprow fails because of bazel. How should I update tidb bazel file?

@Defined2014
Copy link
Contributor

@mjonss @Defined2014 It seems fast_test_tiprow fails because of bazel. How should I update tidb bazel file?

cc @hawkingrei

@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from e58964b to 5cc5872 Compare February 12, 2025 07:13
@L-maple L-maple force-pushed the feature/support_more_types_for_partition_prune branch from 5cc5872 to 48f5dfe Compare February 12, 2025 07:51
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Feb 12, 2025
Copy link

ti-chi-bot bot commented Feb 12, 2025

[LGTM Timeline notifier]

Timeline:

  • 2025-02-07 10:42:38.715625628 +0000 UTC m=+7601.111847687: ☑️ agreed by mjonss.
  • 2025-02-12 07:58:29.296674523 +0000 UTC m=+429751.692896584: ☑️ agreed by Defined2014.

@Defined2014
Copy link
Contributor

/retest

Copy link

ti-chi-bot bot commented Feb 12, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Defined2014, hawkingrei, mjonss

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Feb 12, 2025
@ti-chi-bot ti-chi-bot bot merged commit f87e69b into pingcap:master Feb 12, 2025
24 checks passed
zeminzhou pushed a commit to zeminzhou/tidb that referenced this pull request May 6, 2025
@windtalker
Copy link
Contributor

windtalker commented Aug 1, 2025

Theoretically speaking, if we have an extra function upon the partition column, it is not safe to do the partition prune, for example: if the filter is
func(partition_col) = 1
actually, we can't do partition prune for general function.
More specific speaking, if the partition is a range partition, then we can do partition prune only if func is order-preserving, and clearly, cast is not order-preserving, and this will cause error result, #62754 is an example.
And a more simple example is

mysql> create table t_partition(a char(10)) partition by range columns(a) (PARTITION P0 VALUES LESS THAN ('b'), PARTITION P4 VALUES LESS THAN ('c'), PARTITION PMX VALUES LESS THAN (MAXVALUE));
Query OK, 0 rows affected (0.03 sec)

mysql> create table t_normal(a char(10));
Query OK, 0 rows affected (0.03 sec)
mysql> insert into t_partition values('a'),('b'),('c');
Query OK, 3 rows affected (0.01 sec)
Records: 3  Duplicates: 0  Warnings: 0

mysql> insert into t_normal values('a'),('b'),('c');
Query OK, 3 rows affected (0.00 sec)
Records: 3  Duplicates: 0  Warnings: 0
mysql> explain select * from t_partition where a = 0;
+-------------------------+---------+-----------+-------------------+------------------------------------------------+
| id                      | estRows | task      | access object     | operator info                                  |
+-------------------------+---------+-----------+-------------------+------------------------------------------------+
| TableReader_7           | 2.40    | root      | partition:PMX     | data:Selection_6                               |
| └─Selection_6           | 2.40    | cop[tikv] |                   | eq(cast(test.t_partition.a, double BINARY), 0) |
|   └─TableFullScan_5     | 3.00    | cop[tikv] | table:t_partition | keep order:false, stats:pseudo                 |
+-------------------------+---------+-----------+-------------------+------------------------------------------------+
3 rows in set, 3 warnings (0.01 sec)

mysql>  select * from t_partition where a = 0;
+------+
| a    |
+------+
| c    |
+------+
1 row in set, 3 warnings (0.00 sec)
mysql> explain select * from t_normal where a = 0;
+-------------------------+---------+-----------+----------------+---------------------------------------------+
| id                      | estRows | task      | access object  | operator info                               |
+-------------------------+---------+-----------+----------------+---------------------------------------------+
| TableReader_7           | 2.40    | root      |                | data:Selection_6                            |
| └─Selection_6           | 2.40    | cop[tikv] |                | eq(cast(test.t_normal.a, double BINARY), 0) |
|   └─TableFullScan_5     | 3.00    | cop[tikv] | table:t_normal | keep order:false, stats:pseudo              |
+-------------------------+---------+-----------+----------------+---------------------------------------------+
3 rows in set (0.00 sec)

mysql>  select * from t_normal where a = 0;
+------+
| a    |
+------+
| a    |
| b    |
| c    |
+------+
3 rows in set, 3 warnings (0.00 sec)

@windtalker
Copy link
Contributor

windtalker commented Aug 1, 2025

I suggest we should revert this pr first, and if we still want to do similar optimization, we need a more rigorous implementation @hawkingrei @fixdb cc @L-maple

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved lgtm ok-to-test Indicates a PR is ready to be tested. release-note-none Denotes a PR that doesn't merit a release note. sig/planner SIG: Planner size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

planner: partition prune not work when partition key has a different type with constant
6 participants