-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-32290][SQL] SingleColumn Null Aware Anti Join Optimize #29104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
45fe128
25fcb78
7a02c0b
ec66df6
b00abfe
f338a47
519acd8
eebe303
39c2abe
924ed2b
b964535
d541067
b5831ad
4d26695
1bca273
f2cc24a
cdc04b2
19de983
36b024c
5bfec22
dcccdeb
7824c45
5e050c4
233eff6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2672,6 +2672,17 @@ object SQLConf { | |
| .checkValue(_ >= 0, "The value must be non-negative.") | ||
| .createWithDefault(8) | ||
|
|
||
| val OPTIMIZE_NULL_AWARE_ANTI_JOIN = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You forgot to add |
||
| buildConf("spark.sql.optimizeNullAwareAntiJoin") | ||
| .internal() | ||
| .doc("When true, NULL-aware anti join execution will be planed into " + | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is only possible when we can buildSide data is small. Can you add to this doc?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if it's single column NAAj, even if the buildSide is bigger than the autoBroadcaseThreshold, I think it's better to just using the optimize, because BroadcastNestedLoopJoin will be still worst than this optimization.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw SPARK-32644, so it is not always able to use this optimization, isn't? |
||
| "BroadcastHashJoinExec with flag isNullAwareAntiJoin enabled, " + | ||
| "optimized from O(M*N) calculation into O(M) calculation " + | ||
| "using Hash lookup instead of Looping lookup." + | ||
| "Only support for singleColumn NAAJ for now.") | ||
| .booleanConf | ||
| .createWithDefault(true) | ||
|
|
||
| /** | ||
| * Holds information about keys that have been deprecated. | ||
| * | ||
|
|
@@ -3277,6 +3288,9 @@ class SQLConf extends Serializable with Logging { | |
| def coalesceBucketsInJoinMaxBucketRatio: Int = | ||
| getConf(SQLConf.COALESCE_BUCKETS_IN_JOIN_MAX_BUCKET_RATIO) | ||
|
|
||
| def optimizeNullAwareAntiJoin: Boolean = | ||
| getConf(SQLConf.OPTIMIZE_NULL_AWARE_ANTI_JOIN) | ||
|
|
||
| /** ********************** SQLConf functionality methods ************ */ | ||
|
|
||
| /** Set Spark SQL configuration properties. */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,14 +46,23 @@ case class BroadcastHashJoinExec( | |
| buildSide: BuildSide, | ||
| condition: Option[Expression], | ||
| left: SparkPlan, | ||
| right: SparkPlan) | ||
| right: SparkPlan, | ||
| isNullAwareAntiJoin: Boolean = false) | ||
| extends HashJoin with CodegenSupport { | ||
|
|
||
| if (isNullAwareAntiJoin) { | ||
| require(leftKeys.length == 1, "leftKeys length should be 1") | ||
| require(rightKeys.length == 1, "rightKeys length should be 1") | ||
| require(joinType == LeftAnti, "joinType must be LeftAnti.") | ||
| require(buildSide == BuildRight, "buildSide must be BuildRight.") | ||
| require(condition.isEmpty, "null aware anti join optimize condition should be empty.") | ||
| } | ||
|
|
||
| override lazy val metrics = Map( | ||
| "numOutputRows" -> SQLMetrics.createMetric(sparkContext, "number of output rows")) | ||
|
|
||
| override def requiredChildDistribution: Seq[Distribution] = { | ||
| val mode = HashedRelationBroadcastMode(buildBoundKeys) | ||
| val mode = HashedRelationBroadcastMode(buildBoundKeys, isNullAwareAntiJoin) | ||
| buildSide match { | ||
| case BuildLeft => | ||
| BroadcastDistribution(mode) :: UnspecifiedDistribution :: Nil | ||
|
|
@@ -133,10 +142,37 @@ case class BroadcastHashJoinExec( | |
| val numOutputRows = longMetric("numOutputRows") | ||
|
|
||
| val broadcastRelation = buildPlan.executeBroadcast[HashedRelation]() | ||
| streamedPlan.execute().mapPartitions { streamedIter => | ||
| val hashed = broadcastRelation.value.asReadOnlyCopy() | ||
| TaskContext.get().taskMetrics().incPeakExecutionMemory(hashed.estimatedSize) | ||
| join(streamedIter, hashed, numOutputRows) | ||
| if (isNullAwareAntiJoin) { | ||
| streamedPlan.execute().mapPartitionsInternal { streamedIter => | ||
| val hashed = broadcastRelation.value.asReadOnlyCopy() | ||
| TaskContext.get().taskMetrics().incPeakExecutionMemory(hashed.estimatedSize) | ||
| if (hashed == EmptyHashedRelation) { | ||
| streamedIter | ||
| } else if (hashed == EmptyHashedRelationWithAllNullKeys) { | ||
| Iterator.empty | ||
| } else { | ||
| val keyGenerator = UnsafeProjection.create( | ||
| BindReferences.bindReferences[Expression]( | ||
| leftKeys, | ||
| AttributeSeq(left.output)) | ||
| ) | ||
| streamedIter.filter(row => { | ||
| val lookupKey: UnsafeRow = keyGenerator(row) | ||
| if (lookupKey.anyNull()) { | ||
| false | ||
| } else { | ||
| // Anti Join: Drop the row on the streamed side if it is a match on the build | ||
| hashed.get(lookupKey) == null | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| } else { | ||
| streamedPlan.execute().mapPartitions { streamedIter => | ||
| val hashed = broadcastRelation.value.asReadOnlyCopy() | ||
| TaskContext.get().taskMetrics().incPeakExecutionMemory(hashed.estimatedSize) | ||
|
cloud-fan marked this conversation as resolved.
|
||
| join(streamedIter, hashed, numOutputRows) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -454,6 +490,40 @@ case class BroadcastHashJoinExec( | |
| val (matched, checkCondition, _) = getJoinCondition(ctx, input) | ||
| val numOutput = metricTerm(ctx, "numOutputRows") | ||
|
|
||
| if (isNullAwareAntiJoin) { | ||
| if (broadcastRelation.value == EmptyHashedRelation) { | ||
| return s""" | ||
| |// If the right side is empty, NAAJ simply returns the left side. | ||
| |$numOutput.add(1); | ||
| |${consume(ctx, input)} | ||
| """.stripMargin | ||
| } else if (broadcastRelation.value == EmptyHashedRelationWithAllNullKeys) { | ||
| return s""" | ||
| |// If the right side contains any all-null key, NAAJ simply returns Nothing. | ||
| """.stripMargin | ||
| } else { | ||
| val found = ctx.freshName("found") | ||
| return s""" | ||
| |boolean $found = false; | ||
| |// generate join key for stream side | ||
| |${keyEv.code} | ||
| |if ($anyNull) { | ||
| | $found = true; | ||
| |} else { | ||
| | UnsafeRow $matched = (UnsafeRow)$relationTerm.getValue(${keyEv.value}); | ||
| | if ($matched != null) { | ||
| | $found = true; | ||
| | } | ||
| |} | ||
| | | ||
| |if (!$found) { | ||
| | $numOutput.add(1); | ||
| | ${consume(ctx, input)} | ||
| |} | ||
|
Comment on lines
+506
to
+522
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems we can get rid of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about maybe I could update these code as well with the new HashedRelation Name in next PR. |
||
| """.stripMargin | ||
| } | ||
| } | ||
|
|
||
| if (uniqueKeyCodePath) { | ||
| val found = ctx.freshName("found") | ||
| s""" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -96,7 +96,8 @@ private[execution] object HashedRelation { | |
| input: Iterator[InternalRow], | ||
| key: Seq[Expression], | ||
| sizeEstimate: Int = 64, | ||
| taskMemoryManager: TaskMemoryManager = null): HashedRelation = { | ||
| taskMemoryManager: TaskMemoryManager = null, | ||
| isNullAware: Boolean = false): HashedRelation = { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about renaming this to Or adding a comment about what it truly means, either here or on line 386 below.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| val mm = Option(taskMemoryManager).getOrElse { | ||
| new TaskMemoryManager( | ||
| new UnifiedMemoryManager( | ||
|
|
@@ -107,10 +108,12 @@ private[execution] object HashedRelation { | |
| 0) | ||
| } | ||
|
|
||
| if (key.length == 1 && key.head.dataType == LongType) { | ||
| LongHashedRelation(input, key, sizeEstimate, mm) | ||
| if (isNullAware && !input.hasNext) { | ||
| EmptyHashedRelation | ||
| } else if (key.length == 1 && key.head.dataType == LongType) { | ||
| LongHashedRelation(input, key, sizeEstimate, mm, isNullAware) | ||
| } else { | ||
| UnsafeHashedRelation(input, key, sizeEstimate, mm) | ||
| UnsafeHashedRelation(input, key, sizeEstimate, mm, isNullAware) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the previous version of this PR: I am not sure if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for now, i am using EmptyHashedRelation and EmptyHashedRelationWithAllNullKeys to represent isOriginalInputEmpty, allNullColumnKeyExistsInOriginalInput, isNullAware should be passed into UnsafeHashedRelation and LongHashedRelation, and during building the relation, it might just return a EmptyHashedRelationWithAllNullKeys if a row with all null column key hits. |
||
| } | ||
| } | ||
| } | ||
|
|
@@ -310,7 +313,8 @@ private[joins] object UnsafeHashedRelation { | |
| input: Iterator[InternalRow], | ||
| key: Seq[Expression], | ||
| sizeEstimate: Int, | ||
| taskMemoryManager: TaskMemoryManager): HashedRelation = { | ||
| taskMemoryManager: TaskMemoryManager, | ||
| isNullAware: Boolean = false): HashedRelation = { | ||
|
cloud-fan marked this conversation as resolved.
|
||
|
|
||
| val pageSizeBytes = Option(SparkEnv.get).map(_.memoryManager.pageSizeBytes) | ||
| .getOrElse(new SparkConf().get(BUFFER_PAGESIZE).getOrElse(16L * 1024 * 1024)) | ||
|
|
@@ -338,6 +342,8 @@ private[joins] object UnsafeHashedRelation { | |
| throw new SparkOutOfMemoryError("There is not enough memory to build hash map") | ||
| // scalastyle:on throwerror | ||
| } | ||
| } else if (isNullAware) { | ||
| return EmptyHashedRelationWithAllNullKeys | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -889,7 +895,8 @@ private[joins] object LongHashedRelation { | |
| input: Iterator[InternalRow], | ||
| key: Seq[Expression], | ||
| sizeEstimate: Int, | ||
| taskMemoryManager: TaskMemoryManager): LongHashedRelation = { | ||
| taskMemoryManager: TaskMemoryManager, | ||
| isNullAware: Boolean = false): HashedRelation = { | ||
|
cloud-fan marked this conversation as resolved.
|
||
|
|
||
| val map = new LongToUnsafeRowMap(taskMemoryManager, sizeEstimate) | ||
| val keyGenerator = UnsafeProjection.create(key) | ||
|
|
@@ -903,15 +910,61 @@ private[joins] object LongHashedRelation { | |
| if (!rowKey.isNullAt(0)) { | ||
| val key = rowKey.getLong(0) | ||
| map.append(key, unsafeRow) | ||
| } else if (isNullAware) { | ||
| return EmptyHashedRelationWithAllNullKeys | ||
| } | ||
| } | ||
| map.optimize() | ||
| new LongHashedRelation(numFields, map) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Common trait with dummy implementation for NAAJ special HashedRelation | ||
| * EmptyHashedRelation | ||
| * EmptyHashedRelationWithAllNullKeys | ||
| */ | ||
| trait NullAwareHashedRelation extends HashedRelation with Externalizable { | ||
| override def get(key: InternalRow): Iterator[InternalRow] = { | ||
| throw new UnsupportedOperationException | ||
| } | ||
|
|
||
| override def getValue(key: InternalRow): InternalRow = { | ||
| throw new UnsupportedOperationException | ||
| } | ||
|
|
||
| override def keyIsUnique: Boolean = true | ||
|
|
||
| override def keys(): Iterator[InternalRow] = { | ||
| throw new UnsupportedOperationException | ||
| } | ||
|
|
||
| override def close(): Unit = {} | ||
|
|
||
| override def writeExternal(out: ObjectOutput): Unit = {} | ||
|
|
||
| override def readExternal(in: ObjectInput): Unit = {} | ||
|
|
||
| override def estimatedSize: Long = 0 | ||
| } | ||
|
|
||
| /** | ||
| * A special HashedRelation indicates it built from a empty input:Iterator[InternalRow]. | ||
| */ | ||
| object EmptyHashedRelation extends NullAwareHashedRelation { | ||
| override def asReadOnlyCopy(): EmptyHashedRelation.type = this | ||
| } | ||
|
|
||
| /** | ||
| * A special HashedRelation indicates it built from a non-empty input:Iterator[InternalRow], | ||
| * which contains all null columns key. | ||
| */ | ||
| object EmptyHashedRelationWithAllNullKeys extends NullAwareHashedRelation { | ||
| override def asReadOnlyCopy(): EmptyHashedRelationWithAllNullKeys.type = this | ||
|
Comment on lines
+962
to
+963
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This object name really confuses.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, indeed, but I can't come out with better naming, could you please help with the naming, and i will create a new PR to do code refine, since this PR is closed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably just remove |
||
| } | ||
|
|
||
| /** The HashedRelationBroadcastMode requires that rows are broadcasted as a HashedRelation. */ | ||
| case class HashedRelationBroadcastMode(key: Seq[Expression]) | ||
| case class HashedRelationBroadcastMode(key: Seq[Expression], isNullAware: Boolean = false) | ||
|
cloud-fan marked this conversation as resolved.
|
||
| extends BroadcastMode { | ||
|
|
||
| override def transform(rows: Array[InternalRow]): HashedRelation = { | ||
|
|
@@ -923,9 +976,9 @@ case class HashedRelationBroadcastMode(key: Seq[Expression]) | |
| sizeHint: Option[Long]): HashedRelation = { | ||
| sizeHint match { | ||
| case Some(numRows) => | ||
| HashedRelation(rows, canonicalized.key, numRows.toInt) | ||
| HashedRelation(rows, canonicalized.key, numRows.toInt, isNullAware = isNullAware) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Should this simply be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is still a taskMemoryManager: TaskMemoryManager = null (with default value) just before isNullAware, I've had to using the named argument. ^_^ |
||
| case None => | ||
| HashedRelation(rows, canonicalized.key) | ||
| HashedRelation(rows, canonicalized.key, isNullAware = isNullAware) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,8 @@ | ||
| -- Test filter clause for aggregate expression. | ||
|
|
||
| --CONFIG_DIM1 spark.sql.optimizeNullAwareAntiJoin=true | ||
| --CONFIG_DIM1 spark.sql.optimizeNullAwareAntiJoin=false | ||
|
|
||
|
Comment on lines
+3
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding these. It gives us more confidence.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thanks to @cloud-fan I know of this better way to do e2e case coverage when adding a new feature. |
||
| -- Test data. | ||
| CREATE OR REPLACE TEMPORARY VIEW testData AS SELECT * FROM VALUES | ||
| (1, 1), (1, 2), (2, 1), (2, 2), (3, 1), (3, 2), (null, 1), (3, null), (null, null) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this. Is this covered in the positive/negative test case below ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, added a negative case.