[SPARK-12506][SPARK-12126][SQL]use CatalystScan for JDBCRelation

[huaxingao]

As suggested here, I will change JDBCRelation to implement CatalystScan, and then directly access Catalyst expressions in JDBCRDD.

[AmplabJenkins]

Can one of the admins verify this patch?

[HyukjinKwon]

cc @liancheng

[HyukjinKwon]

It looks you forgot to uncomment those..

[maropu]

Great work though, I think that we should consider lots of things to refactor these kinds of datasource push-down codes because related tickets (SPARK-12449, 12686, ...) describes non-filter logical operators push downs. Also, this pr could break binary compatibility...

[HyukjinKwon]

As @maropu said, for me I also agree with him but some may think differently as mentioned here.

[viirya]

Is PrunedFilteredScan and CatalystScan conflicting? If not (looks they are not), can we keep original JDBCRelation and let it implement CatalystScan too? In order not to duplicate current codes, I am wondering if SQLBuilder can help converting filtering expressions to SQL string.

[maropu]

I agree that CatalystScan is used to support arithmetic operations in datasources though, this current CatalystScan trait only processes filter expressions. If we make this kind of interface changes, it'd be better to consider other requirements discussed in jira; otherwise, we get stuck with the same issue every time we add new features in datasources.

Anyway, the idea to share codes in SQLBuilder and JDBCRDD is good to me.

[rxin]

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.

[kyprifog]

@rxin Why was this pull request closed? Can you direct me to a new one that could have replaced it? I was trying to track down the modification of push downs for basic operations like "limit".

[HyukjinKwon]

Technical reason: It's kind of risky to rely on CatalystScan and completely replace the interface. I think I already see some tests were disabled here. Also, there look potential better suggestions above.

Practical reason: there are too many pending PRs as you see. If the author is not responsive and the PR is inactive to review comments, we better leave them closed for now - seems it's already stuck in few technical reasons. The author is welcome to reopen and other contributors are welcome to take over.

[HyukjinKwon]

BTW, datasource v2 is in progress too to allow more push downs (see SPARK-22386). You might want to take a look

[kyprifog]

@HyukjinKwon Thanks this is what I was looking for. Glad the work is still being continued.