Closed dongyoungy closed 5 years ago
That driver version is also very outdated, the project has also forked to prestosql/presto where the majority of the updates are being done!
On Mon, 10 Jun. 2019, 06:12 Yongjoo Park, notifications@github.com wrote:
@pyongjoo requested changes on this pull request.
Thanks! I just left minor comment regarding pom.xml
In pom.xml https://github.com/mozafari/verdictdb/pull/380#discussion_r291848751:
@@ -300,6 +300,14 @@
1.7
com.facebook.presto presto-jdbc 0.212 It looks like we have been including jdbc drivers only for the test phase. Is there any difference for Presto?
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/mozafari/verdictdb/pull/380?email_source=notifications&email_token=AAIEBCUTJS5TDYVZEHHSYFTPZVP4HA5CNFSM4HWIHK3KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB27TTHA#pullrequestreview-247413148, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIEBCUKNDD2OJUOA5OPJVLPZVP4HANCNFSM4HWIHK3A .
I think we had an internal discussion of 'providing JDBC drivers' vs. 'letting users use their own JDBC' and IIRC, the test-only scope has been commented out as a result. It was commented out in this commit: https://github.com/mozafari/verdictdb/commit/ac65e2c9a6a9f44a135688b1fc97ad5e47553a73
I will update the JDBC version and restore the test scope for now.
After a little bit of digging, it looks likes our PrestoJDBCConnection class requires PrestoStatement
and QueryStats
so that is actually why we removed 'test' scope previously.
Merging #380 into master will increase coverage by
0.1%
. The diff coverage is100%
.
@@ Coverage Diff @@
## master #380 +/- ##
=========================================
+ Coverage 66.43% 66.53% +0.1%
=========================================
Files 169 169
Lines 11635 11635
Branches 1903 1903
=========================================
+ Hits 7729 7740 +11
+ Misses 3397 3387 -10
+ Partials 509 508 -1
Impacted Files | Coverage Δ | |
---|---|---|
...org/verdictdb/connection/PrestoJdbcConnection.java | 36.67% <ø> (ø) |
:arrow_up: |
...ain/java/org/verdictdb/sqlsyntax/PrestoSyntax.java | 13.34% <100%> (ø) |
:arrow_up: |
...or/QueryResultAccuracyEstimatorFromDifference.java | 83.17% <0%> (-0.99%) |
:arrow_down: |
...va/org/verdictdb/coordinator/ExecutionContext.java | 62.13% <0%> (-0.54%) |
:arrow_down: |
.../core/scrambling/FastConvergeScramblingMethod.java | 91.91% <0%> (+0.58%) |
:arrow_up: |
.../verdictdb/core/execplan/ExecutableNodeRunner.java | 88.54% <0%> (+4.75%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 095d0d0...85f7244. Read the comment docs.
I am merging this PR after updating the group id of presto-jdbc to io.prestosql (which should be more up-to-date).
Thanks!
Thanks for this!
(Resolves #377) Add support for the following REGEXP_* functions in Presto as shown in https://prestosql.io/docs/current/functions/regexp.html:
Please note that the lambda expression used in one of the example at the website is not currently supported in VerdictDB.