GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor #38305

wotbrew · 2023-10-17T09:40:34Z

What changes are included in this PR?

Fix incorrect implementation of DenseUnionVector.getBufferSizeFor.
Sum the type id counts before calling getBufferSizeFor on the union's child vectors.

Are these changes tested?

Yes. A test verifies the OOB read (requires bounds check), and an example count is correct, fails as expected before fix.

This PR contains a "Critical Fix".

For users of DenseUnionVector the size can calculated incorrectly, as well as cause of out-of-bounds buffer reads which may return garbage (or potentially segfaulting) if bounds checking is off.

Closes: [Java] Incorrect getBufferSize for a dense union vector #38242

github-actions · 2023-10-17T09:47:47Z

⚠️ GitHub issue #38242 has been automatically assigned in GitHub to PR creator.

pitrou · 2023-10-19T14:04:14Z

cc @jduo

pitrou · 2023-10-19T14:06:27Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

@@ -714,8 +714,22 @@ public int getBufferSizeFor(final int count) {
    if (count == 0) {
      return 0;
    }
+
+    int[] legCounts = new int[typeFields.length];


What is a "leg" here? This is not common terminology in Arrow.

Could call this typeCounts or childCounts.

Forgive the name 😓, internal team term leakage!

Fixed by adapting setChildVectorValueCounts (using counts)

pitrou · 2023-10-19T14:09:07Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+      legCounts[tid]++;
+    }
+
+    long legBytes = 0;


Same here: call this totalChildBytes for example.

pitrou · 2023-10-19T14:18:27Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+    for (ValueVector v : internalStruct) {
+      legBytes += (long)v.getBufferSizeFor(legCounts[tid]);
+      tid++;
+    }


I don't think this is right: internalStruct has its fields indexed by physical child id (that is: 0, 1, 2...) but the type ids are logical values that are unrelated to the physical structure.

But I think that you should just copy what is already done in setChildVectorValueCounts and adapt it for the use case of counting buffer bytes.

pitrou · 2023-10-19T14:20:19Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

    return (int) (count * TYPE_WIDTH + (long) count * OFFSET_WIDTH
-        + DataSizeRoundingUtil.divideBy8Ceil(count) + internalStruct.getBufferSizeFor(count));
+        + DataSizeRoundingUtil.divideBy8Ceil(count) + legBytes);


I think DataSizeRoundingUtil.divideBy8Ceil(count) is a mistake: union vectors don't have a validity bitmap, so they don't need any buffer for it.

pitrou · 2023-10-19T14:21:29Z

java/vector/src/test/java/org/apache/arrow/vector/complex/TestDUVBufferSize.java

+import org.apache.arrow.vector.util.DataSizeRoundingUtil;
+import org.junit.jupiter.api.Test;
+
+public class TestDUVBufferSize {


I'm not sure "DUV" is a common abbreviation, can you just spell it out in full? (also in the file name)

pitrou · 2023-10-19T14:24:40Z

java/vector/src/test/java/org/apache/arrow/vector/complex/TestDUVBufferSize.java

+              new Field("b", FieldType.notNullable(new ArrowType.Binary()), null)
+      );
+
+      duv.initializeChildrenFromFields(fields);


Ok, but that's the easy case as it will use auto-incremented type ids (0,1,2...).
Instead, I would recommend calling DenseUnionVector.addVector which allows you to add child fields with non-trivial type ids (7,6,5 for example).

pitrou · 2023-10-19T14:25:10Z

java/vector/src/test/java/org/apache/arrow/vector/complex/TestDUVBufferSize.java

+      int ac = BaseValueVector.INITIAL_VALUE_ALLOCATION + 1;
+      for (int i = 0; i < ac; i++) {
+        duv.setTypeId(i, (byte) 0);
+        duv.setSafe(i, nih);
+      }
+
+      int bc = 1;
+      for (int i = 0; i < bc; i++) {
+        duv.setTypeId(i + ac, (byte) 1);
+        duv.setSafe(i + ac, nvbh);
+      }
+
+      int count = ac + bc;


Please, can you avoid cryptic variable names? (nih, ac, etc.)

pitrou · 2023-10-19T14:26:07Z

java/vector/src/test/java/org/apache/arrow/vector/complex/TestDUVBufferSize.java

+      duv.setValueCount(count);
+
+      // will not necessarily see an error unless bounds checking is on.
+      assertDoesNotThrow(duv::getBufferSize);


Can we also test getBufferSize with an explicit argument?

…UV getBufferSizeFor (apache#38242)

pitrou · 2023-10-23T11:27:04Z

Thanks a lot for the update @wotbrew ! CI failure is unrelated, so I'll merge.

…enseUnionVector#getBufferSizeFor (apache#38305) ### What changes are included in this PR? Fix incorrect implementation of `DenseUnionVector.getBufferSizeFor`. Sum the type id counts before calling `getBufferSizeFor` on the union's child vectors. ### Are these changes tested? Yes. A test verifies the OOB read (requires bounds check), and an example count is correct, fails as expected before fix. **This PR contains a "Critical Fix".** For users of DenseUnionVector the size can calculated incorrectly, as well as cause of out-of-bounds buffer reads which may return garbage (or potentially segfaulting) if bounds checking is off. * Closes: apache#38242 Authored-by: Dan Stone <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

conbench-apache-arrow · 2023-10-23T17:57:11Z

After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 3793560.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

…enseUnionVector#getBufferSizeFor (apache#38305) ### What changes are included in this PR? Fix incorrect implementation of `DenseUnionVector.getBufferSizeFor`. Sum the type id counts before calling `getBufferSizeFor` on the union's child vectors. ### Are these changes tested? Yes. A test verifies the OOB read (requires bounds check), and an example count is correct, fails as expected before fix. **This PR contains a "Critical Fix".** For users of DenseUnionVector the size can calculated incorrectly, as well as cause of out-of-bounds buffer reads which may return garbage (or potentially segfaulting) if bounds checking is off. * Closes: apache#38242 Authored-by: Dan Stone <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

wotbrew requested a review from lidavidm as a code owner October 17, 2023 09:40

wotbrew force-pushed the 38242-duv-buffer-size branch from 1d115bd to 12c9d04 Compare October 17, 2023 09:43

wotbrew changed the title ~~GH-38242: [Java] Fix incorrect internal struct accounting for DUV getBufferSizeFor (#38242)~~ GH-38242: [Java] Fix incorrect internal struct accounting for DUV getBufferSizeFor Oct 17, 2023

github-actions bot added the awaiting review Awaiting review label Oct 17, 2023

wotbrew force-pushed the 38242-duv-buffer-size branch from 12c9d04 to bb8d171 Compare October 17, 2023 09:50

pitrou requested changes Oct 19, 2023

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Oct 19, 2023

wotbrew force-pushed the 38242-duv-buffer-size branch from bb8d171 to d1a13b1 Compare October 20, 2023 14:00

github-actions bot added the Component: Java label Oct 20, 2023

apacheGH-38242: [Java] Fix incorrect internal struct accounting for D…

680578a

…UV getBufferSizeFor (apache#38242)

wotbrew force-pushed the 38242-duv-buffer-size branch from d1a13b1 to 680578a Compare October 20, 2023 14:07

lidavidm approved these changes Oct 20, 2023

View reviewed changes

lidavidm changed the title ~~GH-38242: [Java] Fix incorrect internal struct accounting for DUV getBufferSizeFor~~ GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor Oct 20, 2023

github-actions bot added awaiting merge Awaiting merge and removed awaiting committer review Awaiting committer review labels Oct 20, 2023

pitrou approved these changes Oct 23, 2023

View reviewed changes

pitrou merged commit 3793560 into apache:main Oct 23, 2023
17 of 18 checks passed

pitrou removed the awaiting merge Awaiting merge label Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor #38305

GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor #38305

wotbrew commented Oct 17, 2023 •

edited by pitrou

Loading

github-actions bot commented Oct 17, 2023

pitrou commented Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

wotbrew Oct 20, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou Oct 19, 2023

pitrou commented Oct 23, 2023

conbench-apache-arrow bot commented Oct 23, 2023

GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor #38305

GH-38242: [Java] Fix incorrect internal struct accounting for DenseUnionVector#getBufferSizeFor #38305

Conversation

wotbrew commented Oct 17, 2023 • edited by pitrou Loading

What changes are included in this PR?

Are these changes tested?

github-actions bot commented Oct 17, 2023

pitrou commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Oct 23, 2023

conbench-apache-arrow bot commented Oct 23, 2023

wotbrew commented Oct 17, 2023 •

edited by pitrou

Loading