lidavidm commented on code in PR #41636:
URL: https://github.com/apache/arrow/pull/41636#discussion_r1599317277
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java:
##########
@@ -450,6 +454,77 @@ protected boolean
compareBaseLargeVariableWidthVectors(Range range) {
return true;
}
+ protected boolean compareBaseVariableWidthViewVectors(Range range) {
+ BaseVariableWidthViewVector leftVector = (BaseVariableWidthViewVector)
left;
+ BaseVariableWidthViewVector rightVector = (BaseVariableWidthViewVector)
right;
+
+ final ArrowBuf leftViewBuffer = leftVector.getDataBuffer();
+ final ArrowBuf rightViewBuffer = rightVector.getDataBuffer();
+
+ final int elementSize = BaseVariableWidthViewVector.ELEMENT_SIZE;
+ final int lengthWidth = BaseVariableWidthViewVector.LENGTH_WIDTH;
+ final int prefixWidth = BaseVariableWidthViewVector.PREFIX_WIDTH;
+ final int bufIndexWidth = BaseVariableWidthViewVector.BUF_INDEX_WIDTH;
+
+ for (int i = 0; i < range.getLength(); i++) {
+ int leftIndex = range.getLeftStart() + i;
+ int rightIndex = range.getRightStart() + i;
+
+ boolean isNull = leftVector.isNull(leftIndex);
+ if (isNull != rightVector.isNull(rightIndex)) {
+ return false;
+ }
+
+ int startIndexLeft = leftIndex * elementSize;
Review Comment:
nit: "index" is being used to mean many different things here. Maybe
"offset" or "byte offset" or "byte index" or "buffer index"?
##########
java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java:
##########
@@ -132,6 +136,24 @@ public void testBaseVariableVectorRangeEquals() {
}
}
+ @Test
+ public void testBaseVariableViewVectorRangeEquals() {
+ try (final ViewVarCharVector vector1 = new ViewVarCharVector("varchar",
allocator);
+ final ViewVarCharVector vector2 = new ViewVarCharVector("varchar",
allocator)) {
+
+ setVector(vector1, STR1, STR2, STR4, STR3, STR2, STR5, STR1, STR6);
+ setVector(vector2, STR1, STR2, STR4, STR3, STR2, STR5, STR1, STR6);
+
+ RangeEqualsVisitor visitor = new RangeEqualsVisitor(vector1, vector2);
+ // inclusion of long string in the middle
+ assertTrue(visitor.rangeEquals(new Range(1, 1, 3)));
+ // inclusion of long string at the start
+ assertTrue(visitor.rangeEquals(new Range(2, 2, 4)));
+ // inclusion of long string at the end
+ assertTrue(visitor.rangeEquals(new Range(4, 4, 4)));
Review Comment:
we should also test nulls, and ranges that should not be equal
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java:
##########
@@ -450,6 +454,77 @@ protected boolean
compareBaseLargeVariableWidthVectors(Range range) {
return true;
}
+ protected boolean compareBaseVariableWidthViewVectors(Range range) {
+ BaseVariableWidthViewVector leftVector = (BaseVariableWidthViewVector)
left;
+ BaseVariableWidthViewVector rightVector = (BaseVariableWidthViewVector)
right;
+
+ final ArrowBuf leftViewBuffer = leftVector.getDataBuffer();
+ final ArrowBuf rightViewBuffer = rightVector.getDataBuffer();
+
+ final int elementSize = BaseVariableWidthViewVector.ELEMENT_SIZE;
+ final int lengthWidth = BaseVariableWidthViewVector.LENGTH_WIDTH;
+ final int prefixWidth = BaseVariableWidthViewVector.PREFIX_WIDTH;
+ final int bufIndexWidth = BaseVariableWidthViewVector.BUF_INDEX_WIDTH;
+
+ for (int i = 0; i < range.getLength(); i++) {
+ int leftIndex = range.getLeftStart() + i;
+ int rightIndex = range.getRightStart() + i;
+
+ boolean isNull = leftVector.isNull(leftIndex);
+ if (isNull != rightVector.isNull(rightIndex)) {
+ return false;
+ }
+
+ int startIndexLeft = leftIndex * elementSize;
+ int endIndexLeft = (leftIndex + 1) * elementSize;
+
+ int startIndexRight = rightIndex * elementSize;
+ int endIndexRight = (rightIndex + 1) * elementSize;
+
+ // check equality of the view
+ int retView = ByteFunctionHelpers.equal(leftViewBuffer, startIndexLeft,
endIndexLeft,
+ rightViewBuffer, startIndexRight, endIndexRight);
+
+ if (retView == 0) {
+ return false;
+ }
+
+ int leftDataBufferValueLength = leftVector.getValueLength(leftIndex);
+ int rightDataBufferValueLength = rightVector.getValueLength(rightIndex);
+
+ if (leftDataBufferValueLength != rightDataBufferValueLength) {
+ return false;
+ }
+
+ // if the value is stored in the dataBuffers
+ if (leftDataBufferValueLength > BaseVariableWidthViewVector.INLINE_SIZE)
{
+ int leftDataBufferIndex = leftViewBuffer.getInt(startIndexLeft +
lengthWidth + prefixWidth);
+ int rightDataBufferIndex = rightViewBuffer.getInt(startIndexRight +
lengthWidth + prefixWidth);
+
+ List<ArrowBuf> leftDataBuffers = leftVector.getDataBuffers();
+ List<ArrowBuf> rightDataBuffers = rightVector.getDataBuffers();
Review Comment:
hoist outside loop
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java:
##########
@@ -450,6 +454,77 @@ protected boolean
compareBaseLargeVariableWidthVectors(Range range) {
return true;
}
+ protected boolean compareBaseVariableWidthViewVectors(Range range) {
+ BaseVariableWidthViewVector leftVector = (BaseVariableWidthViewVector)
left;
+ BaseVariableWidthViewVector rightVector = (BaseVariableWidthViewVector)
right;
+
+ final ArrowBuf leftViewBuffer = leftVector.getDataBuffer();
+ final ArrowBuf rightViewBuffer = rightVector.getDataBuffer();
+
+ final int elementSize = BaseVariableWidthViewVector.ELEMENT_SIZE;
+ final int lengthWidth = BaseVariableWidthViewVector.LENGTH_WIDTH;
+ final int prefixWidth = BaseVariableWidthViewVector.PREFIX_WIDTH;
+ final int bufIndexWidth = BaseVariableWidthViewVector.BUF_INDEX_WIDTH;
+
+ for (int i = 0; i < range.getLength(); i++) {
+ int leftIndex = range.getLeftStart() + i;
+ int rightIndex = range.getRightStart() + i;
+
+ boolean isNull = leftVector.isNull(leftIndex);
+ if (isNull != rightVector.isNull(rightIndex)) {
+ return false;
+ }
+
+ int startIndexLeft = leftIndex * elementSize;
+ int endIndexLeft = (leftIndex + 1) * elementSize;
+
+ int startIndexRight = rightIndex * elementSize;
+ int endIndexRight = (rightIndex + 1) * elementSize;
+
+ // check equality of the view
+ int retView = ByteFunctionHelpers.equal(leftViewBuffer, startIndexLeft,
endIndexLeft,
+ rightViewBuffer, startIndexRight, endIndexRight);
Review Comment:
I think we need to check the length, and then compare the remaining 12 bytes
or compare the data buffers.
##########
java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java:
##########
@@ -450,6 +454,77 @@ protected boolean
compareBaseLargeVariableWidthVectors(Range range) {
return true;
}
+ protected boolean compareBaseVariableWidthViewVectors(Range range) {
+ BaseVariableWidthViewVector leftVector = (BaseVariableWidthViewVector)
left;
+ BaseVariableWidthViewVector rightVector = (BaseVariableWidthViewVector)
right;
+
+ final ArrowBuf leftViewBuffer = leftVector.getDataBuffer();
+ final ArrowBuf rightViewBuffer = rightVector.getDataBuffer();
+
+ final int elementSize = BaseVariableWidthViewVector.ELEMENT_SIZE;
+ final int lengthWidth = BaseVariableWidthViewVector.LENGTH_WIDTH;
+ final int prefixWidth = BaseVariableWidthViewVector.PREFIX_WIDTH;
+ final int bufIndexWidth = BaseVariableWidthViewVector.BUF_INDEX_WIDTH;
+
+ for (int i = 0; i < range.getLength(); i++) {
+ int leftIndex = range.getLeftStart() + i;
+ int rightIndex = range.getRightStart() + i;
+
+ boolean isNull = leftVector.isNull(leftIndex);
+ if (isNull != rightVector.isNull(rightIndex)) {
+ return false;
+ }
+
+ int startIndexLeft = leftIndex * elementSize;
+ int endIndexLeft = (leftIndex + 1) * elementSize;
+
+ int startIndexRight = rightIndex * elementSize;
+ int endIndexRight = (rightIndex + 1) * elementSize;
+
+ // check equality of the view
+ int retView = ByteFunctionHelpers.equal(leftViewBuffer, startIndexLeft,
endIndexLeft,
+ rightViewBuffer, startIndexRight, endIndexRight);
Review Comment:
Why are we comparing all 16 bytes? Or rather, why are we comparing the raw
bytes here at all?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
