Skip to content

[SQL]Update MultiInstanceRelation.scala#1312

Closed
baishuo wants to merge 1 commit intoapache:masterfrom
baishuo:test-1
Closed

[SQL]Update MultiInstanceRelation.scala#1312
baishuo wants to merge 1 commit intoapache:masterfrom
baishuo:test-1

Conversation

@baishuo
Copy link
Contributor

@baishuo baishuo commented Jul 7, 2014

I think if multiAppearance is empty,we can return plan directly

I think if multiAppearance is empty,we can return plan directly
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@baishuo baishuo changed the title Update MultiInstanceRelation.scala [SQL]Update MultiInstanceRelation.scala Jul 8, 2014
@baishuo
Copy link
Contributor Author

baishuo commented Jul 8, 2014

Can one of the admins verify this patch?:)

@marmbrus
Copy link
Contributor

marmbrus commented Jul 8, 2014

I think this patch is correct, but I'm not convinced that we need to make these kind of optimizations at the expense of code complexity / length. I'd imagine the performance benefit of this change is negligible and AFAICT it produces the same result as the original code.

@marmbrus
Copy link
Contributor

marmbrus commented Jul 8, 2014

Also, for future reference there are several stylistic issues:

  • Space between if and (
  • Spaces } else {
  • Space between ) and {

@baishuo
Copy link
Contributor Author

baishuo commented Jul 9, 2014

thank you @marmbrus , close this PR

@baishuo baishuo closed this Jul 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants