Skip to content

[SPARK-21603][SQL][FOLLOW-UP] Use -1 to disable maxLinesPerFunction - #19031

Closed
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-21603-FOLLOWUP-2
Closed

[SPARK-21603][SQL][FOLLOW-UP] Use -1 to disable maxLinesPerFunction#19031
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-21603-FOLLOWUP-2

Conversation

@maropu

@maropumaropu commented Aug 24, 2017

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This pr used -1 to disable maxLinesPerFunction added in #18810 because -1 seems to be more natural to disable this too-long-function optimization (e.g. we couldn't use Int.MaxValue in spark-default.conf).

How was this patch tested?

Modified the existing WholeStageCodegenSuite

@SparkQA

SparkQA commented Aug 24, 2017

Copy link
Copy Markdown

Test build #81055 has finished for PR 19031 at commit 60dc64e.

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

@maropu
maropuforce-pushed the SPARK-21603-FOLLOWUP-2 branch from 60dc64e to 9438655CompareAugust 24, 2017 02:16
@viirya

viirya commented Aug 24, 2017

Copy link
Copy Markdown
Member

I'd prefer using -1 to disable maxLinesPerFunction check like this.

viirya
viirya reviewed Aug 24, 2017
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
"for a single function(8000) divided by 2. Use -1 to disable this.")
.intConf
.checkValue(maxLines => maxLines >= -1, "The maximum must not be a negative integer, -1 to " +
"always activate whole-stage codegen.")

@viiryaviiryaAug 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The maximum must not be a negative integer, except for -1 using to always activate whole-stage codegen.

@maropumaropuAug 24, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

ok

viirya
viirya reviewed Aug 24, 2017
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
"the whole-stage codegen is deactivated for this subtree of the current query plan. " +
"The default value 4000 is the max length of byte code JIT supported " +
"for a single function(8000) divided by 2.")
"The default value 2667 is the max length of byte code JIT supported " +

@viiryaviiryaAug 24, 2017•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

2667? I don't see you change the default value below.

@maropumaropuAug 24, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

missed...

@maropu
maropuforce-pushed the SPARK-21603-FOLLOWUP-2 branch from 8a60378 to a0854adCompareAugust 24, 2017 03:39
@viirya

viirya commented Aug 24, 2017

Copy link
Copy Markdown
Member

LGTM

@SparkQA

SparkQA commented Aug 24, 2017

Copy link
Copy Markdown

Test build #81062 has finished for PR 19031 at commit 9438655.

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

@SparkQA

SparkQA commented Aug 24, 2017

Copy link
Copy Markdown

Test build #81065 has finished for PR 19031 at commit a0854ad.

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

@maropu

maropu commented Aug 24, 2017

Copy link
Copy Markdown
MemberAuthor

ping @gatorsmile

@maropu

maropu commented Aug 25, 2017

Copy link
Copy Markdown
MemberAuthor

ping

@gatorsmile

gatorsmile commented Aug 27, 2017

Copy link
Copy Markdown
Member

This is an internal conf. For the advanced users, we do not encourage them to disable it. If they want to disable it, they can simply set it to a number above 8000. Thus, setting maxLinesPerFunction to -1 is not needed, IMO.

@maropu

maropu commented Aug 28, 2017

Copy link
Copy Markdown
MemberAuthor

ok, I'll close for now. Thanks!

@maropumaropu closed this Aug 28, 2017
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viiryaviiryaviirya 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.

4 participants

@maropu@SparkQA@viirya@gatorsmile