[SPARK-35791][SQL] Release on-going map properly for NULL-aware ANTI join #32939
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @cloud-fan could you help take a look when you have time? Thanks. |
|
Kubernetes integration test unable to build dist. exiting with code: 1 |
|
Test build #139892 has finished for PR 32939 at commit
|
cloud-fan
reviewed
Jun 17, 2021
| @@ -479,6 +479,7 @@ private[joins] object UnsafeHashedRelation { | |||
| throw QueryExecutionErrors.cannotAcquireMemoryToBuildUnsafeHashedRelationError() | |||
| } | |||
| } else if (isNullAware) { | |||
| binaryMap.free() | |||
| return HashedRelationWithAllNullKeys | |||
There was a problem hiding this comment.
shall we return it before creating the binary map?
There was a problem hiding this comment.
@cloud-fan - the NULL-aware ANTI join will be done if there's row with NULL join keys. Note the if (!key.anyNull || allowsNullKey) at line 472. So it will be taking effect when we reading the row with NULL join keys, and this information is not known before creating the binary map
cloud-fan
reviewed
Jun 17, 2021
| @@ -1060,6 +1061,7 @@ private[joins] object LongHashedRelation { | |||
| val key = rowKey.getLong(0) | |||
| map.append(key, unsafeRow) | |||
| } else if (isNullAware) { | |||
| map.free() | |||
| return HashedRelationWithAllNullKeys | |||
cloud-fan
approved these changes
Jun 17, 2021
|
thanks, merging to master/3.1! |
cloud-fan
pushed a commit
that referenced
this pull request
Jun 17, 2021
…join ### What changes were proposed in this pull request? NULL-aware ANTI join (https://issues.apache.org/jira/browse/SPARK-32290) detects NULL join keys during building the map for `HashedRelation`, and will immediately return `HashedRelationWithAllNullKeys` without taking care of the map built already. Before returning `HashedRelationWithAllNullKeys`, the map needs to be freed properly to save memory and keep memory accounting correctly. ### Why are the changes needed? Save memory and keep memory accounting correctly for the join query. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing unit tests introduced in #29104 . Closes #32939 from c21/free-null-aware. Authored-by: Cheng Su <chengsu@fb.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e0d81d9) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
|
Thank you @cloud-fan for quick review! |
flyrain
pushed a commit
to flyrain/spark
that referenced
this pull request
Sep 21, 2021
…join ### What changes were proposed in this pull request? NULL-aware ANTI join (https://issues.apache.org/jira/browse/SPARK-32290) detects NULL join keys during building the map for `HashedRelation`, and will immediately return `HashedRelationWithAllNullKeys` without taking care of the map built already. Before returning `HashedRelationWithAllNullKeys`, the map needs to be freed properly to save memory and keep memory accounting correctly. ### Why are the changes needed? Save memory and keep memory accounting correctly for the join query. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing unit tests introduced in apache#29104 . Closes apache#32939 from c21/free-null-aware. Authored-by: Cheng Su <chengsu@fb.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e0d81d9) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
fishcus
pushed a commit
to fishcus/spark
that referenced
this pull request
Jan 12, 2022
…join ### What changes were proposed in this pull request? NULL-aware ANTI join (https://issues.apache.org/jira/browse/SPARK-32290) detects NULL join keys during building the map for `HashedRelation`, and will immediately return `HashedRelationWithAllNullKeys` without taking care of the map built already. Before returning `HashedRelationWithAllNullKeys`, the map needs to be freed properly to save memory and keep memory accounting correctly. ### Why are the changes needed? Save memory and keep memory accounting correctly for the join query. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing unit tests introduced in apache#29104 . Closes apache#32939 from c21/free-null-aware. Authored-by: Cheng Su <chengsu@fb.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e0d81d9) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
NULL-aware ANTI join (https://issues.apache.org/jira/browse/SPARK-32290) detects NULL join keys during building the map for
HashedRelation, and will immediately returnHashedRelationWithAllNullKeyswithout taking care of the map built already. Before returningHashedRelationWithAllNullKeys, the map needs to be freed properly to save memory and keep memory accounting correctly.Why are the changes needed?
Save memory and keep memory accounting correctly for the join query.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing unit tests introduced in #29104 .