You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/12/28 20:07:48 UTC
[GitHub] [arrow] markjschreiber opened a new pull request, #15106: Short circuit null checks when comparing non null field types
markjschreiber opened a new pull request, #15106:
URL: https://github.com/apache/arrow/pull/15106
Relates to #14844
To avoid expensive checks for null values when the vectors cannot contain nulls, a flag is set when a vector is attached to indicate if a null check is needed when `compare(idx1, idx2)` is called. If it isn't then the call will immediately redirect to `compareNotNull(index1, index2)`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markjschreiber commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
markjschreiber commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1059136058
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
For the cases I have tested the java vectors seem to have a validity buffer even when there can be no nulls. Depending on the `AllocationManager` this is some form of buffer called `EMPTY` which is not null but cannot allocate memory for items. It's not possible the test if the buffer is equal to one of these because they are `private`
However, given that someone might change the implementation one day I will include a check for a `null` validity buffer to catch this case as well.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1368050374
Benchmark runs are scheduled for baseline = 47c1ff18534e52f8f18c5ea2b5cbed65d83151fa and contender = d00f016315408d653b2a46d3fd8922616264ced4. d00f016315408d653b2a46d3fd8922616264ced4 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/2ff14515bae24c3ba621038a2609dee1...6bacc697b792444495eaf0b54d277287/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/6c4e23de1d6440ed9ea1a1d84592e5b1...54b9851c02e1451e806ab90d10ff3e2b/)
[Finished :arrow_down:1.79% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ee420f607f41439e80a51794c3477142...b120607b37564d7c948ccbb79e077801/)
[Finished :arrow_down:0.52% :arrow_up:0.07%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/b8c06b70168b4eef85e965e3b3235e36...8b79fe59e6f5417fa8e51df5363f52d6/)
Buildkite builds:
[Finished] [`d00f0163` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2083)
[Finished] [`d00f0163` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2105)
[Finished] [`d00f0163` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2077)
[Finished] [`d00f0163` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2097)
[Finished] [`47c1ff18` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2082)
[Finished] [`47c1ff18` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2104)
[Finished] [`47c1ff18` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2076)
[Finished] [`47c1ff18` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2096)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1058939428
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
It may be preferable to check for the presence/absence of a validity buffer (I think Java does the same optimization as C++ here) - that'll cover cases where a nullable vector has no nulls, too.
It won't cover cases where a non-nullable vector has a validity buffer of all 1s, but as-is I'd say we may not want to enable this by default since nothing is really verifying that the data matches that part of the schema.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cyb70289 commented on pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
cyb70289 commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1367699243
@markjschreiber ping me if you need CI approval. Most maintainers are in vacation.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1059410649
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
Ah, thanks for working through it. I suppose Java doesn't optimize the validity buffer in the same way that C++ does.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1368050438
['Python', 'R'] benchmarks have high level of regressions.
[ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/ee420f607f41439e80a51794c3477142...b120607b37564d7c948ccbb79e077801/)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15106: Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1366890386
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of old issues on JIRA the title also supports:
ARROW-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - How to contribute patches](https://arrow.apache.org/docs/developers/contributing.html#how-to-contribute-patches)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1366891706
* Closes: #14844
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1059137762
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
Ah, that's what I wasn't remembering. In that case, the right check might be to see if the buffer length is 0 (in which case there can't be any nulls)?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markjschreiber commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
markjschreiber commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1059169767
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
Weirdly it seems that the validity buffer does have a capacity set when any values are added to the value buffer. There is a method to find the number of nulls in the vector which is relatively fast, only needs to be checked when attaching the vector and can be skipped for `FieldTypes` that are `nonNullable`
Turns out I also need to add a safety check that uses the "null safe" comparison when the vector contains no values (valueCount = 0) as the `TreeBasedDictionaryBuilder` seems to need this.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] markjschreiber commented on a diff in pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
markjschreiber commented on code in PR #15106:
URL: https://github.com/apache/arrow/pull/15106#discussion_r1059169767
##########
java/algorithm/src/main/java/org/apache/arrow/algorithm/sort/VectorValueComparator.java:
##########
@@ -76,6 +88,10 @@ public void attachVector(V vector) {
public void attachVectors(V vector1, V vector2) {
this.vector1 = vector1;
this.vector2 = vector2;
+
+ final boolean v1MayHaveNulls = vector1.getField().getFieldType().isNullable();
+ final boolean v2MayHaveNulls = vector2.getField().getFieldType().isNullable();
Review Comment:
Weirdly it seems that the validity buffer does have a capacity set when any values are added to the value buffer. There is a method to find the number of nulls in the vector which is relatively fast, only needs to be checked when attaching the vector and can be skipped for `FieldTypes` that are `nonNullable`
```
private boolean mayHaveNulls(V v) {
return v.getField().getFieldType().isNullable() ||
v.getNullCount() > 0;
}
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15106:
URL: https://github.com/apache/arrow/pull/15106#issuecomment-1366891722
:warning: GitHub issue #14844 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #15106: GH-14844: [Java] Short circuit null checks when comparing non null field types
Posted by GitBox <gi...@apache.org>.
lidavidm merged PR #15106:
URL: https://github.com/apache/arrow/pull/15106
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org