Skip to content

[SPARK-6397][SQL] Check the missingInput simply #5082

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 1 commit into from
Closed

[SPARK-6397][SQL] Check the missingInput simply #5082

wants to merge 1 commit into from

Conversation

@watermen
Copy link
Contributor

@watermen watermen commented Mar 18, 2015

No description provided.

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 18, 2015

Can one of the admins verify this patch?

@the-sea
Copy link

the-sea commented Mar 19, 2015

greate improvement!

@marmbrus
Copy link
Contributor

marmbrus commented Mar 21, 2015

test this please

@SparkQA
Copy link

SparkQA commented Mar 21, 2015

Test build #28947 has started for PR 5082 at commit 25766b9.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Mar 21, 2015

Test build #28947 has finished for PR 5082 at commit 25766b9.

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

@AmplabJenkins
Copy link

AmplabJenkins commented Mar 21, 2015

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/28947/
Test PASSed.

@scwf scwf mentioned this pull request Mar 23, 2015
Closed
@watermen
Copy link
Contributor Author

watermen commented Mar 23, 2015

@liancheng @marmbrus Any more comment on this?

@liancheng
Copy link
Contributor

liancheng commented Mar 23, 2015

LGTM, #5129 also needs this one. I'm merging this to master and branch-1.3. Thanks for working on this!

@asfgit asfgit closed this in e566fe5 Mar 23, 2015
liancheng
liancheng reviewed Mar 23, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicOperators.scala
Copy link
Contributor

@liancheng liancheng Mar 23, 2015

Choose a reason for hiding this comment

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

Why do we prefer adding missingInput in Expand? At first I thought we should just update QueryPlan.missingInput to take the grouping ID virtual column into account.

Copy link
Contributor Author

@watermen watermen Mar 23, 2015

Choose a reason for hiding this comment

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

@liancheng The grouping ID virtual column only used in Expand(LogicalPlan), so I think it's better to add the grouping ID virtual column in subclass(Expand) of QueryPlan. If add missingInput in QueryPlan, it will be more and more when other case like the grouping ID virtual column.

Copy link
Contributor

@liancheng liancheng Mar 23, 2015

Choose a reason for hiding this comment

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

My point is we may also support other virtual columns like INPUT__FILE__NAME and BLOCK__OFFSET__INSIDE__FILE in the future. These virtual columns can be used in more general places like Project and Filter. That's why I think moving this to QueryPlan may make more sense.

@liancheng
Copy link
Contributor

liancheng commented Mar 23, 2015

Sorry, just reverted my last merge, please see my comment above.

@liancheng
Copy link
Contributor

liancheng commented Mar 23, 2015

@watermen A little off topic, would you mind to set your name properly by executing this:

$ git config --global user.name "Yadong Qi"

Currently your name shows as q00251598 in the Git log. This can be inconvenient for us (mostly for @pwendell) to generate the credit list for the next release. Thanks!

@watermen
Copy link
Contributor Author

watermen commented Mar 23, 2015

@liancheng Thanks, I'll do it.

@watermen watermen mentioned this pull request Mar 23, 2015
Closed
@liancheng
Copy link
Contributor

liancheng commented Mar 23, 2015

Since my previous merge operation closed this PR, @watermen has opened #5132 to supersede this.

asfgit pushed a commit that referenced this pull request Mar 23, 2015
@watermen @liancheng
[SPARK-6397][SQL] Check the missingInput simply
a29f493 
#5082

/cc liancheng

Author: Yadong Qi <[email protected]>

Closes #5132 from watermen/sql-missingInput-new and squashes the following commits:

1e5bdc5 [Yadong Qi] Check the missingInput simply

(cherry picked from commit 9f3273b)
Signed-off-by: Cheng Lian <[email protected]>
asfgit pushed a commit that referenced this pull request Mar 23, 2015
@watermen @liancheng
[SPARK-6397][SQL] Check the missingInput simply
9f3273b 
#5082

/cc liancheng

Author: Yadong Qi <[email protected]>

Closes #5132 from watermen/sql-missingInput-new and squashes the following commits:

1e5bdc5 [Yadong Qi] Check the missingInput simply
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@watermen @AmplabJenkins @the-sea @marmbrus @SparkQA @liancheng