Skip to content

[SPARK-24478][SQL][followup] Move projection and filter push down to physical conversion - #21574

Closed
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:followup
Closed

[SPARK-24478][SQL][followup] Move projection and filter push down to physical conversion#21574
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:followup

Conversation

@cloud-fan

@cloud-fancloud-fan commented Jun 15, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

This is a followup of #21503, to completely move operator pushdown to the planner rule.

The code are mostly from #21319

How was this patch tested?

existing tests

@cloud-fan

cloud-fan commented Jun 15, 2018

Copy link
Copy Markdown
ContributorAuthor

cc @rdblue@gengliangwang

cloud-fan
cloud-fan commented Jun 15, 2018
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala Outdated
case _ =>
throw new AnalysisException(s"Data source is not readable: $name")
}
private def asReadSupport: ReadSupport = source match {

@cloud-fancloud-fanJun 15, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

strip one indentation level to shorten the code.

@rdbluerdblueJun 18, 2018

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.

I'm -1 on cosmetic changes like this. It doesn't make a big improvement, but makes it harder to track down when a functional change happened.

@SparkQA

SparkQA commented Jun 15, 2018

Copy link
Copy Markdown

Test build #91885 has finished for PR 21574 at commit 1aeb6ab.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@cloud-fan

cloud-fan commented Jun 15, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented Jun 16, 2018

Copy link
Copy Markdown

Test build #91937 has finished for PR 21574 at commit 1aeb6ab.

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

@rxin

rxin commented Jun 16, 2018

Copy link
Copy Markdown
Contributor

Does this move actually make sense? It'd destroy stats estimation for partition pruning.

@cloud-fan

cloud-fan commented Jun 18, 2018

Copy link
Copy Markdown
ContributorAuthor

According to the dicsussion , we agree that it's better to remove the hacky workaround for mutable DataSourceV2Relation and not having stats estimation for partition pruning. For builtin data source like file source, we can still keep the optimzer rule PruneFileSourcePartitions to make them work.

@rdblue

rdblue commented Jun 18, 2018

Copy link
Copy Markdown
Contributor

@rxin, we can also add the second pushdown (in the stats visitor) to get better stats with a property to turn it on or off. We're going to add it back in our branch anyway.

rdblue
rdblue reviewed Jun 18, 2018
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
*
* @return pushed filter and post-scan filters.
*/
private def pushFilters(

@rdbluerdblueJun 18, 2018

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.

+1 for moving these functions. I considered it in the other commit, but decided to go with fewer changes. I like them here.

rdblue
rdblue reviewed Jun 18, 2018
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala
output: Seq[AttributeReference],
options: Map[String, String],
userSpecifiedSchema: Option[StructType] = None)
userSpecifiedSchema: Option[StructType])

@rdbluerdblueJun 18, 2018

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.

Why is this change necessary?

@cloud-fancloud-fanJun 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I can't find a place that uses this default value.

@rdbluerdblueJun 18, 2018

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.

That's because there are few places that create v2 relations so far, but when SQL statements and other paths that don't allow you to supply your own schema are added, I think this will be more common. It's okay to remove it, but I don't see much value in the change and I like to keep non-functional changes to a minimum.

@cloud-fancloud-fanJun 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

yea I agree we should minimum the non-functional changes, but removing dead code is also good to do. This is really a small change, if we do need the default value in the future, it's very easy to add it back.

@rdbluerdblueJun 18, 2018

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.

Either way, it's up to you.

rdblue
rdblue reviewed Jun 18, 2018
View reviewed changes
Comment thread...core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala Outdated
object DataSourceV2Relation {
private implicit class SourceHelpers(source: DataSourceV2) {
def asReadSupport: ReadSupport = {
private def asReadSupport: ReadSupport = {

@rdbluerdblueJun 18, 2018

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.

Minor: these are effectively private because SourceHelpers is private. If we were to move SourceHelpers or make it public to some other part of Spark, we would have to revert these changes. So I think it is best to rely on the visibility of SourceHelpers instead of making these private.

@cloud-fancloud-fanJun 18, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

makes sense

@rdblue

rdblue commented Jun 18, 2018

Copy link
Copy Markdown
Contributor

+1 (non-binding) assuming that tests pass.

@SparkQA

SparkQA commented Jun 18, 2018

Copy link
Copy Markdown

Test build #92041 has finished for PR 21574 at commit 13c91df.

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

@SparkQA

SparkQA commented Jun 19, 2018

Copy link
Copy Markdown

Test build #92049 has finished for PR 21574 at commit 4471422.

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

@cloud-fan

cloud-fan commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

thanks, merging to master!

jzhuge pushed a commit to jzhuge/spark that referenced this pull request Mar 7, 2019
@cloud-fan@jzhuge
[SPARK-24478][SQL][FOLLOWUP] Move projection and filter push down to …
390b460
…physical conversion
This is a followup of apache#21503, to completely move operator pushdown to the planner rule.
The code are mostly from apache#21319
existing tests
Author: Wenchen Fan <wenchen@databricks.com>
Closesapache#21574 from cloud-fan/followup.
(cherry picked from commit 1737d45)
Conflicts:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
rdblue pushed a commit to rdblue/spark that referenced this pull request Apr 3, 2019
@cloud-fan@rdblue
[SPARK-24478][SQL][FOLLOWUP] Move projection and filter push down to …
9b85abe
…physical conversion
This is a followup of apache#21503, to completely move operator pushdown to the planner rule.
The code are mostly from apache#21319
existing tests
Author: Wenchen Fan <wenchen@databricks.com>
Closesapache#21574 from cloud-fan/followup.
jzhuge pushed a commit to jzhuge/spark that referenced this pull request Oct 15, 2019
@cloud-fan@jzhuge
[SPARK-24478][SQL][FOLLOWUP] Move projection and filter push down to …
dc8529a
…physical conversion
This is a followup of apache#21503, to completely move operator pushdown to the planner rule.
The code are mostly from apache#21319
existing tests
Author: Wenchen Fan <wenchen@databricks.com>
Closesapache#21574 from cloud-fan/followup.
(cherry picked from commit 1737d45)
Conflicts:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Strategy.scala
otterc pushed a commit to linkedin/spark that referenced this pull request Mar 22, 2023
@cloud-fan@otterc
[SPARK-24478][SQL][FOLLOWUP] Move projection and filter push down to …
3d863f3
…physical conversion
This is a followup of apache#21503, to completely move operator pushdown to the planner rule.
The code are mostly from apache#21319
existing tests
Author: Wenchen Fan <wenchen@databricks.com>
Closesapache#21574 from cloud-fan/followup.
Ref: LIHADOOP-48531
RB=1853689
G=superfriends-reviewers
R=zolin,fli,yezhou,mshen,latang
A=
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@cloud-fan@SparkQA@rxin@rdblue