Skip to content

[SPARK-24215][PySpark][Follow Up] Implement eager evaluation for DataFrame APIs in PySpark #21553

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 3 commits into from
Closed

[SPARK-24215][PySpark][Follow Up] Implement eager evaluation for DataFrame APIs in PySpark #21553

wants to merge 3 commits into from

Conversation

xuanyuanking
Copy link
Member

What changes were proposed in this pull request?

Address comments in #21370 and add more test.

How was this patch tested?

Enhance test in pyspark/sql/test.py and DataFrameSuite

@xuanyuanking
Copy link
Member Author

cc @HyukjinKwon @gatorsmile

@SparkQA
Copy link

SparkQA commented Jun 13, 2018

Test build #91766 has finished for PR 21553 at commit 8d33af7.

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

@xuanyuanking
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Jun 13, 2018

Test build #91778 has finished for PR 21553 at commit 8d33af7.

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

gatorsmile
gatorsmile reviewed Jun 13, 2018
View reviewed changes
docs/configuration.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

For SQLConf, we do not need to hard code the conf description here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, thanks, done in afada2b.

gatorsmile
gatorsmile reviewed Jun 13, 2018
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you rewrite these descriptions based on the description I posted in the original PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done in afada2b.

@gatorsmile
Copy link
Member

Could you address the comments in the original PR?

@xuanyuanking
Copy link
Member Author 

Could you address the comments in the original PR?

Thanks, I want take this. Maybe it should be done in another jira and PR, and I should fix all the config hard code in PySpark？

@SparkQA
Copy link

SparkQA commented Jun 15, 2018

Test build #91901 has finished for PR 21553 at commit afada2b.

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

xuanyuanking
xuanyuanking commented Jun 26, 2018
View reviewed changes
python/pyspark/sql/conf.py Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

@gatorsmile I moved all core configs using in pyspark into conf.py here. Please have a look when you have time.
#21370 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for fixing this! Let us do it in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, done in #21648

@xuanyuanking
Copy link
Member Author

@gatorsmile I address the comments in the last commit, but maybe it should be done in a independent PR and Jira?

@SparkQA
Copy link

SparkQA commented Jun 26, 2018

Test build #92344 has finished for PR 21553 at commit d719dfb.

  • This patch fails Python style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class ConfigEntry(object):
  • class SQLConf(object):

felixcheung
felixcheung reviewed Jun 26, 2018
View reviewed changes
docs/configuration.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

we are removing documentation?

Copy link
Member

Choose a reason for hiding this comment

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

SQL Confs are not part of the documentation.

Copy link
Member

Choose a reason for hiding this comment

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

this should be in sql-programming-guide.md right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Follow the SQL configuration, all the description can be shown by spark.sql("SET -v").show(numRows = 200, truncate = false). https://spark.apache.org/docs/latest/configuration.html#spark-sql

viirya
viirya reviewed Jun 27, 2018
View reviewed changes
python/pyspark/sql/conf.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I think this PySpark SQLConf stuff should be done in a separate Jira/PR.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it should be separate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, agree, done in #21648.

HyukjinKwon
HyukjinKwon reviewed Jun 27, 2018
View reviewed changes
python/pyspark/sql/conf.py Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the key. I think current way duplicates a lot of codes in Scala side.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I'm also puzzled by this, cause we also do the register in Scala side. How about just call buildConf on Scala side for theses keys which used only on PySpark? Lets discuss it in #21648

@xuanyuanking
Copy link
Member Author

In the last commit I revert the changes of SQLConf and created a new PR of #21648. Could this follow up PR merged first? Thanks.

@SparkQA
Copy link

SparkQA commented Jun 27, 2018

Test build #92373 has finished for PR 21553 at commit 00ae164.

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

@xuanyuanking
Copy link
Member Author

retest this please

@SparkQA
Copy link

SparkQA commented Jun 27, 2018

Test build #92377 has finished for PR 21553 at commit 00ae164.

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

@gatorsmile
Copy link
Member

LGTM Thanks! Merged to master.

@asfgit asfgit closed this in 6a0b77a Jun 27, 2018
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

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

@felixcheung felixcheung felixcheung left review comments

@gatorsmile gatorsmile gatorsmile 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

@xuanyuanking @SparkQA @gatorsmile @viirya @HyukjinKwon @felixcheung