drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From chunhui-shi <...@git.apache.org>
Subject [GitHub] drill pull request #715: DRILL-5105: remove buffer size checking in getBuffe...
Date Tue, 03 Jan 2017 19:52:43 GMT
Github user chunhui-shi commented on a diff in the pull request:

    --- Diff: exec/vector/src/main/java/org/apache/drill/exec/vector/complex/MapVector.java
    @@ -134,10 +134,6 @@ public int getBufferSizeFor(final int valueCount) {
       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.

View raw message