Skip to content

Conversation

@beliefer
Copy link
Contributor

What changes were proposed in this pull request?

#32513 added the case class CommandResult so as we can eagerly execute command locally. But we forgot to update
isLocal of Dataset.

Why are the changes needed?

Dataset.isLocal should consider CommandResult.

Does this PR introduce any user-facing change?

Yes. If the SQL plan is CommandResult, Dataset.isLocal must return true.

How was this patch tested?

No test.

@github-actions github-actions bot added the SQL label Jun 18, 2021
@SparkQA
Copy link

SparkQA commented Jun 18, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44511/

@SparkQA
Copy link

SparkQA commented Jun 18, 2021

Test build #139984 has finished for PR 32963 at commit f9d1b6f.

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

@dongjoon-hyun
Copy link
Member

cc @cloud-fan

*/
def isLocal: Boolean = logicalPlan.isInstanceOf[LocalRelation]
def isLocal: Boolean = logicalPlan.isInstanceOf[LocalRelation] ||
logicalPlan.isInstanceOf[CommandResult]
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test coverage for this, @beliefer ?

Copy link
Contributor

@cloud-fan cloud-fan Jun 21, 2021

Choose a reason for hiding this comment

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

+1, this API has no test at all, we can add one in DataFrameSuite

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reminder.

@dongjoon-hyun
Copy link
Member

Gentle ping, @beliefer .


test("isLocal should consider CommandResult and LocalRelation") {
withTable("t1") {
val df = sql("CREATE TABLE t USING PARQUET AS SELECT 1 as a")
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use SHOW TABLES which returns data? Then we don't need to create a table and wrap with withTable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@SparkQA
Copy link

SparkQA commented Jun 22, 2021

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44643/

@SparkQA
Copy link

SparkQA commented Jun 22, 2021

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44645/

@SparkQA
Copy link

SparkQA commented Jun 22, 2021

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44645/

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @beliefer and @cloud-fan .

@SparkQA
Copy link

SparkQA commented Jun 22, 2021

Test build #140115 has finished for PR 32963 at commit acc2b69.

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

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 43cd6ca Jun 22, 2021
@beliefer
Copy link
Contributor Author

@dongjoon-hyun @cloud-fan Thanks for review.

@SparkQA
Copy link

SparkQA commented Jun 22, 2021

Test build #140117 has finished for PR 32963 at commit 4b2a055.

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

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants