Skip to content

*: handle signed/unsigned in the partition pruning#15436

Merged
lysu merged 4 commits intopingcap:masterfrom
tiancaiamao:unsigned-partition
Mar 24, 2020
Merged

*: handle signed/unsigned in the partition pruning#15436
lysu merged 4 commits intopingcap:masterfrom
tiancaiamao:unsigned-partition

Conversation

@tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Problem Summary:

The old lessThanData use int64 for data representation.
However, the partition by range less than value can be signed or unsigned.
If it's unsigned, it may be out of the int64 range.

What is changed and how it works?

What's Changed:

  • move makeLessThanData processing to the table creating to reduce allocation
  • handle signed/unsigned in compare
  • reuse the session's parser in expression.ParseSimpleExprsWithNames

How it Works:

Compare function now use an unsigned flag to handle the unsigned case.

Related changes

Check List

Tests

  • Unit test

Release note

@tiancaiamao tiancaiamao requested review from a team as code owners March 17, 2020 15:01
@ghost ghost requested review from a team, XuHuaiyu, eurekaka, qw4990 and winoros and removed request for a team March 17, 2020 15:01
@tiancaiamao
Copy link
Contributor Author

PTAL @lysu @imtbkcat

@tiancaiamao
Copy link
Contributor Author

Some fail cases are range columns partition, we have to merge this one first
#15169

@tiancaiamao
Copy link
Contributor Author

PTAL @lysu @imtbkcat

@tiancaiamao tiancaiamao requested review from imtbkcat and lysu and removed request for XuHuaiyu, eurekaka, qw4990 and winoros March 18, 2020 07:10
pruner := rangePruner{lessThan, col, fn}
pruner := rangePruner{
lessThan: lessThanDataInt{
data: partExpr.ForRangePruning.LessThan,

Choose a reason for hiding this comment

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

Using copy here is better? Copy could avoid race here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's read-only data... free from data race, so the copy is unnecessary.

The LessThan is different from expression.Expression or ast.StmtNode
The visit function modify the node like here:

node.XX = XX.Accept(Visitor)

And the expression is also modified during use (ugly code).

Copy link

@imtbkcat imtbkcat left a comment

Choose a reason for hiding this comment

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

LGTM

@imtbkcat imtbkcat added the status/LGT1 Indicates that a PR has LGTM 1. label Mar 23, 2020
@tiancaiamao
Copy link
Contributor Author

PTAL @lysu

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

rest LGTM

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Mar 24, 2020

Codecov Report

Merging #15436 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #15436   +/-   ##
===========================================
  Coverage   80.7970%   80.7970%           
===========================================
  Files           503        503           
  Lines        136021     136021           
===========================================
  Hits         109901     109901           
  Misses        17687      17687           
  Partials       8433       8433           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/performance

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants