You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "viirya (via GitHub)" <gi...@apache.org> on 2024/02/08 08:55:46 UTC
[PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
viirya opened a new pull request, #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Follow up of #9080.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
When applying join filter, we take filter evaluation array as selection mask to filter output batch. Next, we take the reverse mask to select (streamed) rows to join null rows.
With outer join cases, if filter evaluation has null values resulted in the selection mask, their reverse values are still nulls. Which makes some rows missed in (streamed) rows joined null rows.
We should use `prep_null_mask_filter` to handle nulls in selection mask as false before taking reverse mask from it.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya merged PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1482631795
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1209,7 +1209,14 @@ impl SMJStream {
) {
// The reverse of the selection mask. For the rows not pass join filter above,
// we need to join them (left or right) with null rows for outer joins.
- let not_mask = compute::not(mask)?;
+ let not_mask = if mask.null_count() > 0 {
+ // If the mask contains nulls, we need to use `prep_null_mask_filter` to
+ // handle the nulls in the mask as false.
+ compute::not(&compute::prep_null_mask_filter(mask))?
Review Comment:
Using the test in `sort_merge_join.slt` as example:
```
CREATE TABLE IF NOT EXISTS t1(t1_id INT, t1_name TEXT, t1_int INT) AS VALUES
(11, 'a', 1),
(22, 'b', 2),
(33, 'c', 3),
(44, 'd', 4);
CREATE TABLE IF NOT EXISTS t2(t2_id INT, t2_name TEXT, t2_int INT) AS VALUES
(11, 'z', 3),
(22, 'y', 1),
(44, 'x', 3),
(55, 'w', 3);
```
For the query `SELECT t1_id, t1_int, t2_int FROM t1 LEFT JOIN t2 ON t1_id = t2_id AND t1_int >= t2_int`, `33 3 NULL` is one row to output without join filter. Join filter returns null on this row, so its value in mask selection array is null and the row is not picked.
Here we take reverse mask. As it is null, its reverse mask is still false, but this row should be selected here actually. So we need to call `prep_null_mask_filter` to process the mask before taking reverse array.
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1484779625
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1209,7 +1209,14 @@ impl SMJStream {
) {
// The reverse of the selection mask. For the rows not pass join filter above,
// we need to join them (left or right) with null rows for outer joins.
- let not_mask = compute::not(mask)?;
+ let not_mask = if mask.null_count() > 0 {
+ // If the mask contains nulls, we need to use `prep_null_mask_filter` to
+ // handle the nulls in the mask as false.
Review Comment:
```suggestion
// handle the nulls in the mask as false to produce rows where the mask
// was null itself.
```
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1254,6 +1261,20 @@ impl SMJStream {
// For full join, we also need to output the null joined rows from the buffered side
if matches!(self.join_type, JoinType::Full) {
+ // Handle not mask for buffered side further.
+ // For buffered side, we want to output the rows that are not null joined with
+ // the streamed side. i.e. the rows that are not null in the `buffered_indices`.
+ let not_mask = if buffered_indices.null_count() > 0 {
Review Comment:
You might be able to avoid the unwrap by using
```rust
let not_mask = if let Some(nulls) = if buffered_indices.nulls() {
let mask = not_mask.values() & nulls.inner();
BooleanArray::new(mask, None)
} else {
not_mask
};
```
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1482622085
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1209,7 +1209,14 @@ impl SMJStream {
) {
// The reverse of the selection mask. For the rows not pass join filter above,
// we need to join them (left or right) with null rows for outer joins.
- let not_mask = compute::not(mask)?;
+ let not_mask = if mask.null_count() > 0 {
Review Comment:
I ran the added test by #9080 again in a new laptop and found this bug. I'm not sure why previously the test passed locally and in CI in #9080. 🤔
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#issuecomment-1937183713
For anyone following along, @viirya did just that in https://github.com/apache/arrow-datafusion/pull/9191
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1482641685
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1254,6 +1261,20 @@ impl SMJStream {
// For full join, we also need to output the null joined rows from the buffered side
if matches!(self.join_type, JoinType::Full) {
+ // Handle not mask for buffered side further.
+ // For buffered side, we want to output the rows that are not null joined with
+ // the streamed side. i.e. the rows that are not null in the `buffered_indices`.
+ let not_mask = if buffered_indices.null_count() > 0 {
+ let nulls = buffered_indices.nulls().unwrap();
+ let mask = not_mask.values() & nulls.inner();
+ BooleanArray::new(mask, None)
Review Comment:
For full outer join, we need to output buffered rows that fail join filter. But in the `output_batch` batch, we only care about the rows with `buffered_indices` not null. Other rows with null indices are rows failed with equijoin predicates.
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#issuecomment-1937104492
Thank you @alamb
> Maybe it is time to increase the priority of running the CI tests on a mac: https://github.com/apache/arrow-datafusion/issues/9117
Yea, I agreed that we should increase our test coverage to aarch64.
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1482641685
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1254,6 +1261,20 @@ impl SMJStream {
// For full join, we also need to output the null joined rows from the buffered side
if matches!(self.join_type, JoinType::Full) {
+ // Handle not mask for buffered side further.
+ // For buffered side, we want to output the rows that are not null joined with
+ // the streamed side. i.e. the rows that are not null in the `buffered_indices`.
+ let not_mask = if buffered_indices.null_count() > 0 {
+ let nulls = buffered_indices.nulls().unwrap();
+ let mask = not_mask.values() & nulls.inner();
+ BooleanArray::new(mask, None)
Review Comment:
For full outer join, we need to output buffered rows that fail join filter. But in the `output_batch` batch, we only care about the rows with `buffered_indices` not null.
--
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
Re: [PR] Use prep_null_mask_filter to handle nulls in selection mask [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #9163:
URL: https://github.com/apache/arrow-datafusion/pull/9163#discussion_r1483114234
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -1209,7 +1209,14 @@ impl SMJStream {
) {
// The reverse of the selection mask. For the rows not pass join filter above,
// we need to join them (left or right) with null rows for outer joins.
- let not_mask = compute::not(mask)?;
+ let not_mask = if mask.null_count() > 0 {
Review Comment:
I re-checked the results in `sort_merge_join.slt` and it should be correct (as it is same as `join.slt` which is produced by hash join operator).
--
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