Skip to content

[SPARK-22848][SQL] Eliminate mutable state from Stack - #20035

Closed
kiszk wants to merge 4 commits into
apache:masterfrom
kiszk:SPARK-22848
Closed

[SPARK-22848][SQL] Eliminate mutable state from Stack#20035
kiszk wants to merge 4 commits into
apache:masterfrom
kiszk:SPARK-22848

Conversation

@kiszk

@kiszkkiszk commented Dec 20, 2017•
edited
Loading

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This PR eliminates mutable states from the generated code for Stack.

How was this patch tested?

Existing test suites

kiszk
kiszk commented Dec 20, 2017
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/generators.scala
s"""
|$code
|$wrapperClass<InternalRow> ${ev.value} = $wrappedArray;
""".stripMargin, isNull = "false")

@kiszkkiszkDec 20, 2017•
edited
Loading

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.

This change does not use inline = true at ctx.addMutableState for correct code generation.

@kiszkkiszk changed the title Enable to execute generated code of function in Generator[SPARK-22848][SQL] Enable to execute generated code of function in GeneratorDec 20, 2017
@kiszkkiszk changed the title [SPARK-22848][SQL] Enable to execute generated code of function in Generator[SPARK-22848][SQL] Eliminate mutable state from StackDec 20, 2017
@kiszk

kiszk commented Dec 20, 2017

Copy link
Copy Markdown
MemberAuthor

This PR comes from this discussion.

@SparkQA

SparkQA commented Dec 20, 2017

Copy link
Copy Markdown

Test build #85189 has finished for PR 20035 at commit 2b65856.

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

cloud-fan
cloud-fan reviewed Dec 20, 2017
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/generators.scala Outdated
ev.copy(code = code, isNull = "false")
ev.copy(code =
s"""
|InternalRow[] $rowData = new InternalRow[$numRows];

@cloud-fancloud-fanDec 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this creates a large array every time, and I don't think we have data copy issues for generator expressions...

@kiszkkiszkDec 20, 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.

I see. I did not imagine that numRows is large.
I will revert the code for rowData.

@SparkQA

SparkQA commented Dec 20, 2017

Copy link
Copy Markdown

Test build #85191 has finished for PR 20035 at commit 81306c4.

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

@SparkQA

SparkQA commented Dec 20, 2017

Copy link
Copy Markdown

Test build #85194 has finished for PR 20035 at commit d31ccd7.

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

@SparkQA

SparkQA commented Dec 20, 2017

Copy link
Copy Markdown

Test build #85198 has finished for PR 20035 at commit f0163e7.

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

@kiszk

kiszk commented Dec 21, 2017

Copy link
Copy Markdown
MemberAuthor

Jenkins, retest this please

@cloud-fan

cloud-fan commented Dec 21, 2017

Copy link
Copy Markdown
Contributor

LGTM

@SparkQA

SparkQA commented Dec 21, 2017

Copy link
Copy Markdown

Test build #85223 has finished for PR 20035 at commit f0163e7.

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

@viirya

viirya commented Dec 21, 2017

Copy link
Copy Markdown
Member

LGTM

@kiszk

kiszk commented Dec 21, 2017

Copy link
Copy Markdown
MemberAuthor

Jenkins, retest this please

@viirya

viirya commented Dec 21, 2017

Copy link
Copy Markdown
Member

I think the test failure is not related to this change, but the ongoing work to upgrade pyarrow.

@cloud-fan

cloud-fan commented Dec 21, 2017

Copy link
Copy Markdown
Contributor

yea it's failing globally, I'm merging this PR, thanks!

@SparkQA

SparkQA commented Dec 21, 2017

Copy link
Copy Markdown

Test build #85237 has finished for PR 20035 at commit f0163e7.

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

@viirya

viirya commented Dec 21, 2017•
edited
Loading

Copy link
Copy Markdown
Member

I ran the test (HDFSMetadataLogSuite) locally. It should be fine as it passes.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-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.

4 participants

@kiszk@SparkQA@cloud-fan@viirya