Skip to content
This repository has been archived by the owner on Dec 28, 2017. It is now read-only.

Optimize DAG executor building logic with column pruning #203

Merged
merged 6 commits into from
Dec 27, 2017
Merged

Optimize DAG executor building logic with column pruning #203

merged 6 commits into from
Dec 27, 2017

Conversation

Novemser
Copy link
Contributor

@Novemser Novemser commented Dec 18, 2017 •
edited
Loading

Modify executors' build logic, now can push down much less column and get the same result.

This change is Reviewable

@Novemser Novemser self-assigned this Dec 18, 2017
@Novemser Novemser mentioned this pull request Dec 18, 2017
Merged
@Novemser
Copy link
Contributor Author

TPCH:
All passed.

DAG:
Result: Tests run: 6744 Tests succeeded: 3397 Tests failed: 16 Tests skipped: 3331

@Novemser
Copy link
Contributor Author

Refer to pingcap/tispark#143

@Novemser
Copy link
Contributor Author

PTAL @ilovesoup @zhexuany @birdstorm

@zhexuany
Copy link
Contributor

zhexuany commented Dec 25, 2017 •
edited
Loading

is this PR fixes #163

@Novemser
Copy link
Contributor Author

＠zhexuany This PR may not fix #163, in #163 it's plan generation's issue, not executor building logic's issue.

@zhexuany
Copy link
Contributor

zhexuany commented Dec 26, 2017 •
edited
Loading

Thanks. I am trying to figure out which issue this PR can fix. Could you reference that issue in this PR?
Or this is just optimization?

zhexuany
zhexuany reviewed Dec 26, 2017
View reviewed changes
src/main/java/com/pingcap/tikv/meta/TiDAGRequest.java Outdated
@@ -187,7 +188,11 @@ private DAGRequest buildTableScan() {
TableScan.Builder tblScanBuilder = TableScan.newBuilder();

// Step1. Add columns to first executor
tableInfo.getColumns().forEach(tiColumnInfo -> tblScanBuilder.addColumns(tiColumnInfo.toProto(tableInfo)));
getFields().forEach(tiColumnInfo ->
Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

src/main/java/com/pingcap/tikv/meta/TiDAGRequest.java
@@ -264,13 +278,38 @@ private DAGRequest buildTableScan() {
return request;
}

private void setColumnOffsets(TiExpr expr) {
Copy link
Contributor

@zhexuany zhexuany Dec 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

by seting column offset, tikv can take advantage of it. But my concern is where is the logic in tidb side, I just want to make sure this logic can do the trick.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may lie in TiDB's schema(may hard to find in source code...) or TiKV-Client developers' document.

@Novemser
Copy link
Contributor Author

@zhexuany Updated according to comment.

@Novemser
Copy link
Contributor Author

TPCH:
Result: Tests run: 21 Tests succeeded: 21 Tests failed: 0 Tests skipped: 0

DAG:
Result: Tests run: 7335 Tests succeeded: 3767 Tests failed: 8 Tests skipped: 3560

@Novemser
Copy link
Contributor Author

Novemser commented Dec 27, 2017 •
edited
Loading

This optimization should fix #195 and may hugely impact TiKV performance.

zhexuany
zhexuany approved these changes Dec 27, 2017
View reviewed changes
Copy link
Contributor

@zhexuany zhexuany left a comment

Choose a reason for hiding this comment

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

LGTM

@zhexuany zhexuany merged commit f7a4ebd into master Dec 27, 2017
@zhexuany zhexuany deleted the optimize_table_scan_executor branch December 27, 2017 09:33
@Novemser Novemser mentioned this pull request Dec 30, 2017
Closed
This was referenced Jan 15, 2018
Merged
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zhexuany zhexuany zhexuany approved these changes

Assignees

@Novemser Novemser

Labels
reviewable test passed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Novemser @zhexuany