-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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
[SPARK-22508][SQL] Fix 64KB JVM bytecode limit problem with GenerateUnsafeRowJoiner.create() #19737
Conversation
Test build #83801 has finished for PR 19737 at commit
|
|
||
val functions = mutable.ArrayBuffer.empty[String] | ||
val args = "java.lang.Object obj1, long offset1, java.lang.Object obj2, long offset2" | ||
val copyBitsets = splitPlatformCode(copyBitset).zipWithIndex.map { case(body, index) => |
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.
why can't we use splitExpressions
here?
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.
We do not have ctx
here.
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.
then create one. The benefit is code reuse and the nested class optimization
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.
Good idea, I forgot that we can create it. I do not want to reinvent wheels :)
@@ -166,6 +214,8 @@ object GenerateUnsafeRowJoiner extends CodeGenerator[(StructType, StructType), U | |||
| private byte[] buf = new byte[64]; | |||
| private UnsafeRow out = new UnsafeRow(${schema1.size + schema2.size}); | |||
| | |||
| ${functions.mkString("\n")} |
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.
this can be ${ctx.declareAddedFunctions()}
if we create a codegen context.
Test build #84031 has finished for PR 19737 at commit
|
Jenkins, retest this please |
s"$putLong(buf, ${offset + i * 8}, $bits);\n" | ||
} | ||
|
||
val functions = mutable.ArrayBuffer.empty[String] |
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.
not needed?
} | ||
|
||
val functions = mutable.ArrayBuffer.empty[String] | ||
val args = "java.lang.Object obj1, long offset1, java.lang.Object obj2, long offset2" |
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.
ditto
@@ -154,7 +164,10 @@ object GenerateUnsafeRowJoiner extends CodeGenerator[(StructType, StructType), U | |||
|$putLong(buf, $cursor, $getLong(buf, $cursor) + ($shift << 32)); | |||
""".stripMargin |
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.
nit: s"$putLong(buf, $cursor, $getLong(buf, $cursor) + ($shift << 32));\n"
Test build #84037 has finished for PR 19737 at commit
|
val args = "java.lang.Object obj1, long offset1, java.lang.Object obj2, long offset2" | ||
val copyBitsets = ctx.splitExpressions(copyBitset, "copyBitsetFunc", | ||
("java.lang.Object", "obj1") :: ("long", "offset1") :: | ||
("java.lang.Object", "obj2") :: ("long", "offset2") :: Nil) |
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.
Could you use the named arguments when calling splitExpressions
, like what I did in #19790?
} | ||
|
||
val updateOffsets = ctx.splitExpressions(updateOffset, "copyBitsetFunc", | ||
("long", "numBytesVariableRow1") :: Nil) |
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.
The same here.
LGTM pending jenkins |
Test build #84054 has finished for PR 19737 at commit
|
retest this please |
Test build #84061 has finished for PR 19737 at commit
|
thanks, merging to master/2.2! |
…nsafeRowJoiner.create() ## What changes were proposed in this pull request? This PR changes `GenerateUnsafeRowJoiner.create()` code generation to place generated code for statements to operate bitmap and offset into separated methods if these size could be large. ## How was this patch tested? Added a new test case into `GenerateUnsafeRowJoinerSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes #19737 from kiszk/SPARK-22508. (cherry picked from commit c957714) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
…nsafeRowJoiner.create() ## What changes were proposed in this pull request? This PR changes `GenerateUnsafeRowJoiner.create()` code generation to place generated code for statements to operate bitmap and offset into separated methods if these size could be large. ## How was this patch tested? Added a new test case into `GenerateUnsafeRowJoinerSuite` Author: Kazuaki Ishizaki <ishizaki@jp.ibm.com> Closes apache#19737 from kiszk/SPARK-22508. (cherry picked from commit c957714) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
This PR changes
GenerateUnsafeRowJoiner.create()
code generation to place generated code for statements to operate bitmap and offset into separated methods if these size could be large.How was this patch tested?
Added a new test case into
GenerateUnsafeRowJoinerSuite