-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-21362][SQL][Adding Apache Drill JDBC Dialect] #18607
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
Conversation
|
I'm very new to this but was just wondering if there is anything else I should do to help with this pull request or does it just take more time? |
|
Test build #3842 has finished for PR 18607 at commit
|
|
Could you also add the docker-based test suite, like what we did in https://github.com/apache/spark/pull/9893/files? |
|
|
||
| package org.apache.spark.sql.jdbc | ||
|
|
||
| import java.sql.Types |
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.
Do we use this import?
|
|
||
| import java.sql.Types | ||
|
|
||
| import org.apache.spark.sql.types.{BooleanType, DataType, LongType, MetadataBuilder} |
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.
ditto.
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.
These imports are not used, I will remove them. I'll also look at adding the docker tests suggested by @gatorsmile
|
@viira can you review for me again please |
|
@radford1 Have you added the test as suggested by @gatorsmile? |
|
@viirya I have not had a chance to build out the docker tests like @gatorsmile suggested. Will that be a requirement in order to merge it? |
|
@radford1 Yes. That is needed for merging this PR. Could you check it? Thanks! |
|
Unfortunately I think drill is not popular enough to warrant inclusion in here yet. If this is not extensible, we should make it possible to include such mappings outside Spark and then perhaps Drill can just package this (or any 3rd party on github). |
|
@gatorsmile Understood I'll work on including them. |
|
We can just put this code in a 3rd party library, can't we? If there is an issue with service/code discovery, we can come up with some sort of registration process similar to the data source API. |
|
Could we please close this PR? Thanks! |
What changes were proposed in this pull request?
Adding Apache Drill to the JDBC Dialect
How was this patch tested?
I tested this manually by building and using RStudio to connect to the spark server and running a query against an apache drill cluster
the contribution is my original work and that I license the work to the project under the project’s open source license
Please review http://spark.apache.org/contributing.html before opening a pull request.