[SPARK-24288][SQL] Add a JDBC Option to enable preventing predicate pushdown #21875
Conversation
|
Thanks! :) LGTM except two minor comments |
| val sessionInitStatement = parameters.get(JDBC_SESSION_INIT_STATEMENT) | ||
|
|
||
| // An option to allow/disallow pushing down predicate into JDBC data source | ||
| val pushDownPredicate = parameters.getOrElse(JDBC_PUSHDOWN_PREDICATE, "true").toBoolean |
There was a problem hiding this comment.
Super Nit: Shouldn't it be in plural, pushDownPredicates? There may be many predicates
There was a problem hiding this comment.
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 PushDownPredicate and the singular form in similar rules. So maybe we keep it singular here too?
There was a problem hiding this comment.
Yeah, consistency is a very good argument :) Indeed it will be plural or not, depending from which side we are looking at it
| // 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.
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 filters -- unhandledFilters in JdbcRelation.buildScan
There was a problem hiding this comment.
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.
I see now, my mistake. Thanks for clarification :)
There was a problem hiding this comment.
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.
Indeed it's confusing. buildScan argument may be named pushedFilters, variables also, then code will be self-describing
|
Can you add JDBC to the title? |
maryannxue
changed the title
|
Test build #93562 has finished for PR 21875 at commit
|
|
@maryannxue We need to update the programming guide ? |
|
Programming guide updated. Thank you, @dilipbiswal and @HyukjinKwon! |
|
@maryannxue Thanks. It looks good to me. As a minor comment, could we state the default value for this parameter as well ? For some of the other parameters, we specify the default value. |
|
Test build #93579 has finished for PR 21875 at commit
|
|
Test build #93584 has finished for PR 21875 at commit
|
|
retest this please |
|
Test build #93647 has finished for PR 21875 at commit
|
|
Thanks! Merged to master. |
7 participants
What changes were proposed in this pull request?
Add a JDBC Option "pushDownPredicate" (default
true) to allow/disallow predicate push-down in JDBC data source.How was this patch tested?
Add a test in
JDBCSuite