GH-43966: [Java] Check for nullabilities when comparing StructVector #43968

hellishfire · 2024-09-05T08:59:57Z

Rationale for this change

See #43966

What changes are included in this PR?

Check for nullabilities when comparing StructVector with RangeEqualsVisitor.

Are these changes tested?

Yes

Are there any user-facing changes?

No

GitHub Issue: [Java] Check for nullabilities when comparing StructVector #43966

github-actions · 2024-09-05T09:00:27Z

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

vibhatha · 2024-09-05T09:32:58Z

@hellishfire let's make the PR title as same as the issue title.

vibhatha · 2024-09-05T09:35:47Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

@@ -354,6 +355,18 @@ protected boolean compareStructVectors(Range range) {
      return false;
    }

+    if (leftVector instanceof StructVector || rightVector instanceof StructVector) {


What if we do this check for all vectors not just StructVector?

Another thing is this visitor interface is used for all vectors, and specifying a logic just for a single vector type doesn't make much sense. Further, if we are to check the range considering nulls, it should be for all vectors?

@vibhatha
There's already null comparison for other types of vector, as in other compareXXXVectors methods.
The instanceof check here is to see whether either struct vector is nullable. If neither is nullable, there's no need to compare nullability.

Oh, I misinterpreted.

hellishfire · 2024-09-05T11:02:56Z

@hellishfire let's make the PR title as same as the issue title.

I renamed the issue title to be the same as PR title

vibhatha · 2024-09-05T14:28:58Z

Overall LGTM!

vibhatha

Thanks for making this PR.

vibhatha · 2024-09-06T02:31:30Z

@lidavidm appreciate your review and also please approve the workflows.

lidavidm

Do null slots still compare equal when the child values are different? I'd like to see a unit test for that. (They should, but it looks like we just directly dispatch to RangeEqualsVisitor.)

hellishfire · 2024-09-06T03:23:33Z

Do null slots still compare equal when the child values are different? I'd like to see a unit test for that. (They should, but it looks like we just directly dispatch to RangeEqualsVisitor.)

Currently not. Further code change is needed to achieve this.

vibhatha · 2024-09-06T03:24:22Z

Probably we should address that as well. I think it is in scope. Sorry I missed that.

hellishfire · 2024-09-06T06:33:23Z

@vibhatha @lidavidm
I rewrote the compare logic and related tests

java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java

vibhatha · 2024-09-06T08:16:45Z

java/vector/src/test/java/org/apache/arrow/vector/compare/TestRangeEqualsVisitor.java

      writeStructVector(writer2, 2, 20L);
      writeStructVector(writer2, 3, 30L);
      writeStructVector(writer2, 4, 40L);
      writeStructVector(writer2, 0, 0L);
      writer2.setValueCount(5);

      RangeEqualsVisitor visitor = new RangeEqualsVisitor(vector1, vector2);
-      assertTrue(visitor.rangeEquals(new Range(1, 1, 3)));
+      assertTrue(visitor.rangeEquals(new Range(2, 1, 3)));


Maybe write a new test to evaluate the new case?

conbench-apache-arrow · 2024-09-09T15:07:47Z

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

There were 7 benchmark results indicating a performance regression:

Commit Run on arm64-t4g-2xlarge-linux at 2024-09-09 09:03:14Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-21, scale_factor=1
Commit Run on amd64-m5-4xlarge-linux at 2024-09-09 08:54:41Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-01, scale_factor=10
and 5 more (see the report linked below)

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

…ector (apache#43968) ### Rationale for this change See apache#43966 ### What changes are included in this PR? Check for nullabilities when comparing StructVector with RangeEqualsVisitor. ### Are these changes tested? Yes ### Are there any user-facing changes? No * GitHub Issue: apache#43966 Authored-by: youming.whl <[email protected]> Signed-off-by: David Li <[email protected]>

hellishfire requested a review from lidavidm as a code owner September 5, 2024 08:59

github-actions bot added Component: Java awaiting review Awaiting review labels Sep 5, 2024

vibhatha reviewed Sep 5, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Sep 5, 2024

vibhatha approved these changes Sep 5, 2024

View reviewed changes

lidavidm reviewed Sep 6, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Sep 6, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 6, 2024

GH-43966: [Java] Check for nullabilities when comparing StructVector

93b2899

vibhatha reviewed Sep 6, 2024

View reviewed changes

lidavidm approved these changes Sep 9, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Sep 9, 2024

lidavidm merged commit d0f9f3e into apache:main Sep 9, 2024
16 of 17 checks passed

lidavidm removed the awaiting merge Awaiting merge label Sep 9, 2024

lidavidm mentioned this pull request Sep 9, 2024

[Java] Check for nullabilities when comparing StructVector #43966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-43966: [Java] Check for nullabilities when comparing StructVector #43968

GH-43966: [Java] Check for nullabilities when comparing StructVector #43968

hellishfire commented Sep 5, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Sep 5, 2024

vibhatha commented Sep 5, 2024

vibhatha Sep 5, 2024

vibhatha Sep 5, 2024

hellishfire Sep 5, 2024 •

edited

Loading

vibhatha Sep 5, 2024

hellishfire commented Sep 5, 2024 •

edited

Loading

vibhatha commented Sep 5, 2024

vibhatha left a comment

vibhatha commented Sep 6, 2024

lidavidm left a comment

hellishfire commented Sep 6, 2024

vibhatha commented Sep 6, 2024

hellishfire commented Sep 6, 2024

vibhatha Sep 6, 2024

conbench-apache-arrow bot commented Sep 9, 2024

GH-43966: [Java] Check for nullabilities when comparing StructVector #43968

GH-43966: [Java] Check for nullabilities when comparing StructVector #43968

Conversation

hellishfire commented Sep 5, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Sep 5, 2024

vibhatha commented Sep 5, 2024

vibhatha Sep 5, 2024

Choose a reason for hiding this comment

vibhatha Sep 5, 2024

Choose a reason for hiding this comment

hellishfire Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

vibhatha Sep 5, 2024

Choose a reason for hiding this comment

hellishfire commented Sep 5, 2024 • edited Loading

vibhatha commented Sep 5, 2024

vibhatha left a comment

Choose a reason for hiding this comment

vibhatha commented Sep 6, 2024

lidavidm left a comment

Choose a reason for hiding this comment

hellishfire commented Sep 6, 2024

vibhatha commented Sep 6, 2024

hellishfire commented Sep 6, 2024

vibhatha Sep 6, 2024

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Sep 9, 2024

hellishfire commented Sep 5, 2024 •

edited by github-actions bot

Loading

hellishfire Sep 5, 2024 •

edited

Loading

hellishfire commented Sep 5, 2024 •

edited

Loading