You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by ppadma <gi...@git.apache.org> on 2017/03/29 03:15:28 UTC
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
GitHub user ppadma opened a pull request:
https://github.com/apache/drill/pull/802
DRILL-5394: Optimize query planning for MapR-DB tables by caching row\u2026
\u2026 counts
During the planning phase, we fetch MapR DB table row count multiple times. This is an expensive operation and fetching it multiple times increases planning time considerably. Implemented per query cache. Fetch table rowCount only once, cache it in hbaseScanSpec and reuse it. Copy the rowCount from old to new hbaseScanSpec when a new groupScan is created i.e. when the filter is pushed down into scan.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ppadma/drill DRILL-5394
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/802.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #802
----
commit 8ff826c58f074ba58d1ee1b888b5da41eeb2d894
Author: Padma Penumarthy <pp...@yahoo.com>
Date: 2017-03-16T18:40:13Z
DRILL-5394: Optimize query planning for MapR-DB tables by caching row counts
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/802
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by Agirish <gi...@git.apache.org>.
Github user Agirish commented on the issue:
https://github.com/apache/drill/pull/802
Can confirm that with this fix, query times have improved (details will be updated in JIRA). Thanks for making this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/802
The new changes look good! Thanks for further refactoring the code. LGTM +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on the issue:
https://github.com/apache/drill/pull/802
Further code refactoring may be worth the effort - We leave tableStats as-is and pass it around instead of using the new `rowCount`. This would cleanup the code further and would make it more robust to future changes in tableStats.
However, in the interest of time I would not hold up the pull request for the above code refactoring.
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on the issue:
https://github.com/apache/drill/pull/802
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/802
ok, I changed it so we cache MapRDBTableStats, not just rowCount. Please review the new diffs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill issue #802: DRILL-5394: Optimize query planning for MapR-DB tables by ...
Posted by ppadma <gi...@git.apache.org>.
Github user ppadma commented on the issue:
https://github.com/apache/drill/pull/802
Thanks for the review Gautam. Good suggestion. I changed the code. rowCount is cached in groupScan. Added a new constructor to pass the rowCount from old groupScan to new groupScan. Please review the new diffs.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/802#discussion_r108823495
--- Diff: contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/MapRDBPushFilterIntoScan.java ---
@@ -184,6 +184,8 @@ protected void doPushFilterIntoBinaryGroupScan(final RelOptRuleCall call,
return; //no filter pushdown ==> No transformation.
}
+ // Set rowCount in newScanSpec so we do not go and fetch rowCount (an expensive operation) again from MapR DB client.
+ newScanSpec.setRowCount(groupScan.getHBaseScanSpec().getRowCount());
--- End diff --
Change the constructor for BinaryTableGroupScan to also pass in TableStats and add a getter for TableStats. This should be sufficient for passing around the rows.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/802#discussion_r108821978
--- Diff: contrib/storage-hbase/src/main/java/org/apache/drill/exec/store/hbase/HBaseScanSpec.java ---
@@ -76,6 +78,10 @@ public String getTableName() {
return stopRow == null ? HConstants.EMPTY_START_ROW : stopRow;
}
+ public long getRowCount() { return rowCount; }
+
+ public void setRowCount(long numRowCount) { rowCount = numRowCount; }
--- End diff --
Should the ScanSpec be mutable since it serves a specification? I looked at other ScanSpecs but none seem to set ScanSpec members. Maybe all the members should be final and initialized via the constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/802#discussion_r108822541
--- Diff: contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/binary/BinaryTableGroupScan.java ---
@@ -174,7 +174,7 @@ public MapRDBSubScan getSpecificScan(int minorFragmentId) {
@Override
public ScanStats getScanStats() {
//TODO: look at stats for this.
- long rowCount = (long) ((hbaseScanSpec.getFilter() != null ? .5 : 1) * tableStats.getNumRows());
+ long rowCount = (long) ((hbaseScanSpec.getFilter() != null ? .5 : 1) * hbaseScanSpec.getRowCount());
--- End diff --
This code change would not be required if we use the alternative approach?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #802: DRILL-5394: Optimize query planning for MapR-DB tab...
Posted by gparai <gi...@git.apache.org>.
Github user gparai commented on a diff in the pull request:
https://github.com/apache/drill/pull/802#discussion_r108822233
--- Diff: contrib/format-maprdb/src/main/java/org/apache/drill/exec/store/mapr/db/binary/BinaryTableGroupScan.java ---
@@ -115,8 +112,11 @@ private void init() {
try (Admin admin = formatPlugin.getConnection().getAdmin();
RegionLocator locator = formatPlugin.getConnection().getRegionLocator(tableName)) {
hTableDesc = admin.getTableDescriptor(tableName);
- tableStats = new MapRDBTableStats(getHBaseConf(), hbaseScanSpec.getTableName());
-
+ // Fetch rowCount only once and cache it in hbaseScanSpec.
+ if (hbaseScanSpec.getRowCount() == hbaseScanSpec.ROW_COUNT_UNKNOWN) {
+ MapRDBTableStats tableStats = new MapRDBTableStats(getHBaseConf(), hbaseScanSpec.getTableName());
+ hbaseScanSpec.setRowCount(tableStats.getNumRows());
--- End diff --
This looks weird. We create the TableStats relying on some information from the ScanSpec and then proceed to modify the same ScanSpec with the information retrieved from TableStats. Please look at below comment as well regarding Spec mutability.
Should we instead just overload the MapRDBTableStats constructor to allow passing numRows - since that is what the existing constructor endup doing but makes a call to DB Client? So instead of populating the ScanSpec we populate the tableStats using this new constructor?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---