Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 24, 2020

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 eurekaka, winoros, qw4990, XuHuaiyu and a team 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 lysu and imtbkcat and removed request for eurekaka, winoros, qw4990 and XuHuaiyu 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

expression/simple_rewriter.go Outdated Show resolved Hide resolved
expression/simple_rewriter.go Outdated Show resolved Hide resolved
@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