New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ARROW-4532: [Java] fix bug causing very large varchar value buffers #3613

Closed

pravindra wants to merge 2 commits into apache:master from pravindra:varchar

Contributor

pravindra commented Feb 11, 2019 •

edited

Loading

The varchar/varbinary vectors have a setSafe variant that takes an arrow buf and a start/end offsets. The method needs to expand the buffer to make space for 'end - start' bytes. but, due to a bug, it was expanding it to accommodate 'end' bytes, thereby making the value buffer much larger than required.


          ARROW-4532: [Java] fix a size compute bug

8f88e3a

siddharthteotia approved these changes

View reviewed changes

Contributor

siddharthteotia commented Feb 11, 2019

may be edit the commit message slightly to describe the problem?

Member

wesm commented Feb 11, 2019

Yes. It helps for the changelog when other developers can see when a bug they run into was fixed

pravindra changed the title ~~ARROW-4532: [Java] fix a size compute bug~~ ARROW-4532: [Java] fix bug causing very large varchar value buffers

emkornfield reviewed

View reviewed changes

Contributor

emkornfield left a comment

These comments are style that might help somebody new to the code base ramp up quicker (i.e. me) so take them with a grain of salt.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java Outdated

+                    for (int i = 0; i < numValues; i++) {
+                      int start = fromVector.getstartOffset(i);
+                      int end = fromVector.getstartOffset(i + 1);
+                      toVector.setSafe(i, 1, start, end, fromDataBuffer);

Contributor

emkornfield Feb 12, 2019

making this a named constant or putting in comments what the value represents might make the test easier to read.

Contributor Author

pravindra Feb 12, 2019

done

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java Outdated

+                    }
+                    fromVector.setValueCount(numValues);
+                    ArrowBuf fromDataBuffer = fromVector.getDataBuffer();
+                    assertEquals(BaseAllocator.nextPowerOfTwo(numValues * 11), fromDataBuffer.capacity());

Contributor

emkornfield Feb 12, 2019

this test seems like it is potentially brittle if the allocator expansion algorithm was changed. Is this an intrinsic property of this method? Maybe make this an inequality?

Contributor Author

pravindra Feb 12, 2019

fixed.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java Outdated

+                    fromVector.setInitialCapacity(numValues);
+                    fromVector.allocateNew();
+                    for (int i = 0; i < numValues; ++i) {
+                      fromVector.setSafe(i, "hello world".getBytes(), 0, 11);

Contributor

emkornfield Feb 12, 2019

it might make the test easier to read if 0 and 11 where constants or had comments next to them.

Contributor Author

pravindra Feb 12, 2019

fixed.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java

+                      int end = fromVector.getstartOffset(i + 1);
+                      toVector.setSafe(i, 1, start, end, fromDataBuffer);
+                    }
+                    assertEquals(fromDataBuffer.capacity(), toVector.getDataBuffer().capacity());

Contributor

emkornfield Feb 12, 2019

it might pay to add a comment why the capacity is expected to be equal

Contributor Author

pravindra Feb 12, 2019

done.

Contributor Author

pravindra Feb 12, 2019

added.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java

+                          MinorType.VARCHAR, allocator);
+                      final VarCharVector toVector = newVector(VarCharVector.class, EMPTY_SCHEMA_PATH,
+                          MinorType.VARCHAR, allocator)) {
+                    fromVector.setInitialCapacity(numValues);

Contributor

emkornfield Feb 12, 2019

it might make the unit test easier to read if you separated each section by a comment.
i.e.
//Setup

// Execute

// Verify

Contributor Author

pravindra Feb 12, 2019

i've added comments.

java/vector/src/test/java/org/apache/arrow/vector/TestValueVector.java Outdated

@@ @@ -1394,6 +1394,35 @@ public void testFillEmptiesNotOverfill() { @@
                   }
                 }
+                @Test
+                public void testSetSafeWithArrowBuf() {

Contributor

emkornfield Feb 12, 2019

maybe add to the title doesntAllocateExcessiveMemory

Contributor Author

pravindra Feb 12, 2019

done


          ARROW-4532: fix review comments

1b4f224

Contributor

emkornfield commented Feb 12, 2019

@pravindra thanks!

codecov-io commented Feb 12, 2019

Codecov Report

Merging #3613 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3613      +/-   ##
==========================================
- Coverage   87.78%   87.77%   -0.01%     
==========================================
  Files         673      673              
  Lines       82776    82839      +63     
  Branches     1069     1069              
==========================================
+ Hits        72661    72713      +52     
- Misses      10004    10011       +7     
- Partials      111      115       +4

Impacted Files	Coverage Δ
go/arrow/math/int64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/memory/memory_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/float64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/math/uint64_avx2_amd64.go	`0% <0%> (-100%)`	⬇️
go/arrow/memory/memory_amd64.go	`28.57% <0%> (-14.29%)`	⬇️
go/arrow/math/math_amd64.go	`31.57% <0%> (-5.27%)`	⬇️
js/src/visitor/indexof.ts	`97.26% <0%> (-1.37%)`	⬇️
cpp/src/arrow/python/decimal.cc	`98.03% <0%> (-0.85%)`	⬇️
cpp/src/plasma/store.cc	`91.85% <0%> (-0.34%)`	⬇️
go/arrow/math/float64_amd64.go	`33.33% <0%> (ø)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7b6ca7...1b4f224. Read the comment docs.

xhochy approved these changes

View reviewed changes

Member

xhochy left a comment

+1, LGTM

Thanks @pravindra for the comments, they are really good for understanding what is going on!

xhochy closed this in

61b2926

tanyaschlusser pushed a commit to tanyaschlusser/arrow that referenced this pull request


          ARROW-4532: [Java] fix bug causing very large varchar value buffers

f81a43c

The varchar/varbinary vectors have a setSafe variant that takes an arrow buf and a start/end offsets. The method needs to expand the buffer to make space for 'end - start' bytes. but, due to a bug, it was expanding it to accommodate 'end' bytes, thereby making the value buffer much larger than required.

Author: Pindikura Ravindra <ravindra@dremio.com>

Closes apache#3613 from pravindra/varchar and squashes the following commits:

1b4f224 <Pindikura Ravindra> ARROW-4532: fix review comments
8f88e3a <Pindikura Ravindra> ARROW-4532:  fix a size compute bug

asfimport mentioned this pull request

[Java] varchar value buffer much larger than expected #21081

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment