You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by chunhui-shi <gi...@git.apache.org> on 2017/01/01 09:27:58 UTC
[GitHub] drill pull request #715: DRILL-5105: remove buffer size checking in getBuffe...
GitHub user chunhui-shi opened a pull request:
https://github.com/apache/drill/pull/715
DRILL-5105: remove buffer size checking in getBuffers since it is onl\u2026
\u2026y for debug and the redundancy increases exponentially.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/chunhui-shi/drill DRILL-5105
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/715.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 #715
----
commit 81417bb66804733c1193b36cf04164f9463fbd2c
Author: chunhui-shi <cs...@maprtech.com>
Date: 2017-01-01T08:39:46Z
DRILL-5105: remove buffer size checking in getBuffers since it is only for debug and the redundancy increases exponentially.
----
---
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 #715: DRILL-5105: remove buffer size checking in getBuffers sinc...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/715
Keep original code as commented out per offline discussion with Paul.
---
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 #715: DRILL-5105: remove buffer size checking in getBuffe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/715
---
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 #715: DRILL-5105: remove buffer size checking in getBuffers sinc...
Posted by parthchandra <gi...@git.apache.org>.
Github user parthchandra commented on the issue:
https://github.com/apache/drill/pull/715
+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 pull request #715: DRILL-5105: remove buffer size checking in getBuffe...
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/715#discussion_r94471881
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java ---
@@ -134,10 +134,6 @@ public int getBufferSizeFor(final int valueCount) {
@Override
public DrillBuf[] getBuffers(boolean clear) {
--- End diff --
Thanks for suggesting to use assert. It is a nice way to avoid doing this check in production. My thoughts is we don't need any check here, the reasons are:
1, mapVector does not have its own 'real' data vectors but just go to underlying vectors and get a sum of buffer sizes.
2, for non-mapVectors, there may be multiple vectors(offsets, bits, etc, such as ListVector) or the getBufferSize() is just simply get writeIndex, for which super.getBufferSize() is identical to this.getBufferSize(). And if there is any issues that non-mapVector did not calculate bufferSize correctly, we should have already seen in using that specific vector, thus we don't need to do it in mapVector code.
Actually my first thought was rewrite the logic about how to recursively doing bufferSize check. But after I read all getBufferSize on all vectors, I decided the check is not needed at all. Since any error in getBufferSize should immediately cause serious problem(serialize and deserialize) and can not pass the functional tests of that specific vector.
---
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 #715: DRILL-5105: remove buffer size checking in getBuffe...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/715#discussion_r94288743
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java ---
@@ -134,10 +134,6 @@ public int getBufferSizeFor(final int valueCount) {
@Override
public DrillBuf[] getBuffers(boolean clear) {
--- End diff --
perhaps replace with:
```
assert getBufferSize() == super.getBufferSize();
```
We still get the benefit of the check in tests; but the assert is ignored in production runs. This is, in fact, the advantage of the Java `assert` statement over the Guava `Preconditions` methods.
---
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 #715: DRILL-5105: remove buffer size checking in getBuffe...
Posted by paul-rogers <gi...@git.apache.org>.
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/715#discussion_r94288748
--- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/complex/RepeatedMapVector.java ---
@@ -413,14 +413,9 @@ public RepeatedMapAccessor getAccessor() {
@Override
public DrillBuf[] getBuffers(boolean clear) {
--- End diff --
Same comment as above.
---
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.
---