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 [email protected] or file a JIRA ticket
with INFRA.
---
