Skip to content

[SPARK-18016][SQL][FOLLOW-UP] Code Generation: Constant Pool Limit - reduce entries for mutable state #20036

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[SPARK-18016][SQL][FOLLOW-UP] Code Generation: Constant Pool Limit - reduce entries for mutable state #20036

wants to merge 4 commits into from

Conversation

@kiszk
Copy link
Member

@kiszk kiszk commented Dec 20, 2017

What changes were proposed in this pull request?

This PR addresses additional review comments in #19811

How was this patch tested?

Existing test suites

@SparkQA
Copy link

SparkQA commented Dec 20, 2017

Test build #85196 has finished for PR 20036 at commit 53661eb.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Dec 21, 2017

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85224 has finished for PR 20036 at commit 53661eb.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Dec 21, 2017

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85236 has finished for PR 20036 at commit 53661eb.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Dec 21, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Dec 21, 2017

Test build #85260 has finished for PR 20036 at commit 53661eb.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

ueshin
ueshin reviewed Dec 22, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala Outdated
// inline mutable state since not many Like operations in a task
val pattern = ctx.addMutableState(patternClass, "patternLike",
v => s"""$v = ${patternClass}.compile("$regexStr");""", forceInline = true)
v => s"""$v = ${patternClass}.compile("$regexStr");""")
Copy link
Member

@ueshin ueshin Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can remove { and } around patternClass.

Copy link
Member Author

@kiszk kiszk Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, done for other places, too.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala Outdated
// inline mutable state since not many RLike operations in a task
val pattern = ctx.addMutableState(patternClass, "patternRLike",
v => s"""$v = ${patternClass}.compile("$regexStr");""", forceInline = true)
v => s"""$v = ${patternClass}.compile("$regexStr");""")
Copy link
Member

@ueshin ueshin Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

@SparkQA
Copy link

SparkQA commented Dec 22, 2017

Test build #85300 has finished for PR 20036 at commit 8d8b669.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Dec 22, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Dec 22, 2017

Test build #85301 has finished for PR 20036 at commit 8d8b669.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Dec 22, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Dec 22, 2017

Test build #85307 has finished for PR 20036 at commit 8d8b669.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@kiszk
Copy link
Member Author

kiszk commented Dec 26, 2017

ping @cloud-fan

cloud-fan
cloud-fan reviewed Dec 27, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala Outdated
override def doProduce(ctx: CodegenContext): String = {
// Right now, InputAdapter is only used when there is one input RDD.
// inline mutable state since an inputAdaptor in a task
// inline mutable state since an InputAdapter in a task
Copy link
Contributor

@cloud-fan cloud-fan Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you miss some words...

Copy link
Member Author

@kiszk kiszk Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, done

cloud-fan
cloud-fan reviewed Dec 27, 2017
View reviewed changes
...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
// It's possible that we add same mutable state twice, e.g. the `mergeExpressions` in
// `TypedAggregateExpression`, we should call `distinct` here to remove the duplicated ones.
val initCodes = mutableStateInitCode.distinct
val initCodes = mutableStateInitCode.distinct.map(_ + "\n")
Copy link
Contributor

@cloud-fan cloud-fan Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, good catch!

@cloud-fan
Copy link
Contributor

cloud-fan commented Dec 27, 2017

LGTM

@SparkQA
Copy link

SparkQA commented Dec 27, 2017

Test build #85423 has finished for PR 20036 at commit 05da9d7.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

viirya
viirya reviewed Dec 27, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala Outdated
override def doProduce(ctx: CodegenContext): String = {
// Right now, InputAdapter is only used when there is one input RDD.
// inline mutable state since an inputAdaptor in a task
// inline mutable state since an InputAdapter is used once in a task for WholeStageCodegen
Copy link
Member

@viirya viirya Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: inline -> Inline

viirya
viirya reviewed Dec 27, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/joins/SortMergeJoinExec.scala Outdated
val spillThreshold = getSpillThreshold
val inMemoryThreshold = getInMemoryThreshold

// inline mutable state since not many join operations in a task
Copy link
Member

@viirya viirya Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Inline

viirya
viirya reviewed Dec 27, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala Outdated
fastHashMapClassName, groupingKeySchema, bufferSchema).generate()
ctx.addInnerClass(generatedMap)

// inline mutable state since not many aggregation operations in a task
Copy link
Member

@viirya viirya Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Inline

viirya
viirya reviewed Dec 27, 2017
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/HashAggregateExec.scala Outdated
fastHashMapClassName, groupingKeySchema, bufferSchema).generate()
ctx.addInnerClass(generatedMap)

// inline mutable state since not many aggregation operations in a task
Copy link
Member

@viirya viirya Dec 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Inline

@viirya
Copy link
Member

viirya commented Dec 27, 2017

LGTM

@SparkQA
Copy link

SparkQA commented Dec 27, 2017

Test build #85430 has finished for PR 20036 at commit 4ef81c8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan
Copy link
Contributor

cloud-fan commented Dec 28, 2017

thanks, merging to master!

@asfgit asfgit closed this in 5683984 Dec 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viirya viirya viirya left review comments

@ueshin ueshin ueshin left review comments

@cloud-fan cloud-fan cloud-fan left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@kiszk @SparkQA @HyukjinKwon @cloud-fan @viirya @ueshin