-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-24288][SQL] Add a JDBC Option to enable preventing predicate pushdown #21875
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -172,7 +172,11 @@ private[sql] case class JDBCRelation( | |
|
|
||
| // Check if JDBCRDD.compileFilter can accept input filters | ||
| override def unhandledFilters(filters: Array[Filter]): Array[Filter] = { | ||
| filters.filter(JDBCRDD.compileFilter(_, JdbcDialects.get(jdbcOptions.url)).isEmpty) | ||
| if (jdbcOptions.pushDownPredicate) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure, that this is the only place? JDBCRDD.scanTable defines filters as all filters that may be pushed down. Probably we should use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is the only source of truth for defining handled/unhandled. The caller (physical rules) calls this method and push "handled" to JdbcRelation.buildScan. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see now, my mistake. Thanks for clarification :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I share your opinion actually. It is confusing here... maybe we should change the parameter names at some point. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed it's confusing. |
||
| filters.filter(JDBCRDD.compileFilter(_, JdbcDialects.get(jdbcOptions.url)).isEmpty) | ||
| } else { | ||
| filters | ||
| } | ||
| } | ||
|
|
||
| override def buildScan(requiredColumns: Array[String], filters: Array[Filter]): RDD[Row] = { | ||
|
|
||
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.
Super Nit: Shouldn't it be in plural, pushDownPredicates? There may be many predicates
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.
Or one could argue that "predicate" is a notion of all filters as a whole. It's a nice reminder though. I had not thought about it, but anyway I just checked: we use
PushDownPredicateand the singular form in similar rules. So maybe we keep it singular here too?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.
Yeah, consistency is a very good argument :) Indeed it will be plural or not, depending from which side we are looking at it