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.
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
[SPARK-31027] [SQL] Refactor DataSourceStrategy to be more extendable #27778
Conversation
DataSourceStrategy.scala
to minimize the changes to support nested predicate pushdown
...core/src/test/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategySuite.scala
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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, @dbtsai .
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
...core/src/test/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategySuite.scala
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one minor comment.
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
@HyukjinKwon @dongjoon-hyun @viirya @rdblue @cloud-fan Some changes based on @HyukjinKwon 's suggestion to use extractor pattern have been done. Would like to have your final reviews. Thanks, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks okay to me.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
retest this please |
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/DataSourceStrategy.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except a few minor comments
This comment has been minimized.
This comment has been minimized.
Test build #119293 has finished for PR 27778 at commit
|
Merged to master. |
Thank you all. Looks much better! |
### What changes were proposed in this pull request? Refactor `DataSourceStrategy.scala` and `DataSourceStrategySuite.scala` so it's more extendable to implement nested predicate pushdown. ### Why are the changes needed? To support nested predicate pushdown. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing tests and new tests. Closes #27778 from dbtsai/SPARK-31027. Authored-by: DB Tsai <d_tsai@apple.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>
Also merged into 3.0 branch since it's just refactoring without introducing new feature. It will make code maintenance easier for 3.0 and future master. Thanks all for reviewing. |
### What changes were proposed in this pull request? Refactor `DataSourceStrategy.scala` and `DataSourceStrategySuite.scala` so it's more extendable to implement nested predicate pushdown. ### Why are the changes needed? To support nested predicate pushdown. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing tests and new tests. Closes apache#27778 from dbtsai/SPARK-31027. Authored-by: DB Tsai <d_tsai@apple.com> Signed-off-by: HyukjinKwon <gurwls223@apache.org>
What changes were proposed in this pull request?
Refactor
DataSourceStrategy.scala
andDataSourceStrategySuite.scala
so it's more extendable to implement nested predicate pushdown.Why are the changes needed?
To support nested predicate pushdown.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests and new tests.