You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by dsbos <gi...@git.apache.org> on 2015/09/03 23:52:59 UTC
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
GitHub user dsbos opened a pull request:
https://github.com/apache/drill/pull/143
DRILL-3566: Fix: PreparedStatement.executeQuery() got ClassCastException.
Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing. (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)
Added basic test of querying via PreparedStatement. [PreparedStatementTest]
Added some case test of statement-creation methods. [ConnectionTest]
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/dsbos/incubator-drill bugs/drill-3566
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/143.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #143
----
commit 1a870538c66fa59070facce7e2a4342d9869b51e
Author: dbarclay <db...@maprtech.com>
Date: 2015-07-28T02:27:50Z
DRILL-3566: Fix: PreparedStatement.executeQuery() got ClassCastException.
Main:
Restored DrillResultSetImpl(...)'s statement parameter from overly
restrictive DrillStatementImpl to AvaticaStatement and removed caller
cast that was throwing. (Relatedly, adjusted getStatement() and moved
internal casting from statement to connection.)
Added basic test of querying via PreparedStatement. [PreparedStatementTest]
Added some case test of statement-creation methods. [ConnectionTest]
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38706036
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -87,12 +88,12 @@
boolean hasPendingCancelationNotification;
- DrillResultSetImpl(DrillStatementImpl statement, AvaticaPrepareResult prepareResult,
+ DrillResultSetImpl(AvaticaStatement statement, AvaticaPrepareResult prepareResult,
ResultSetMetaData resultSetMetaData, TimeZone timeZone) {
super(statement, prepareResult, resultSetMetaData, timeZone);
- this.statement = statement;
+ connection = (DrillConnectionImpl) statement.getConnection();
final int batchQueueThrottlingThreshold =
- this.getStatement().getConnection().getClient().getConfig().getInt(
+ connection.getClient().getConfig().getInt(
--- End diff --
if you move the line `this.client = client;` before this line you should be able to reuse `this.client` instead of calling `connection.getClient()`
You can also reuse `this.connection` instead of creating another instance `c` below
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38705073
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillJdbc41Factory.java ---
@@ -103,9 +103,9 @@ public DrillResultSetImpl newResultSet(AvaticaStatement statement,
TimeZone timeZone) {
final ResultSetMetaData metaData =
newResultSetMetaData(statement, prepareResult.getColumnList());
- return new DrillResultSetImpl( (DrillStatementImpl) statement,
- (DrillPrepareResult) prepareResult,
- metaData, timeZone);
+ return new DrillResultSetImpl(statement,
+ (DrillPrepareResult) prepareResult,
--- End diff --
Cast here is redundant here, DrillResultSetImpl() is expecting an AvaticaPrepareResult object
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on the pull request:
https://github.com/apache/drill/pull/143#issuecomment-137794418
> can you confirm that this patch passes all our "jdbc" tests ?
Yes, it passes our regular tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on the pull request:
https://github.com/apache/drill/pull/143#issuecomment-137607549
> can you confirm that this patch passes all our "jdbc" tests ?
It passed before the post-review update commit. I'm re-running regular tests now.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38706070
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -1334,8 +1335,6 @@ public String getQueryId() throws SQLException {
@Override
protected DrillResultSetImpl execute() throws SQLException{
- DrillConnectionImpl connection = (DrillConnectionImpl) statement.getConnection();
-
connection.getClient().runQuery(QueryType.SQL, this.prepareResult.getSql(),
--- End diff --
why not use `this.client` instead of calling `connection.getClient()` ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38709955
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -850,9 +851,9 @@ public void moveToCurrentRow() throws SQLException {
}
@Override
- public DrillStatementImpl getStatement() {
+ public AvaticaStatement getStatement() {
--- End diff --
One reason for leaving it in (after changing the return type back) is to hold that comment pointing out that this method doesn't call checkNotClosed() as most other methods do.
Fixed copy/paste/forgot-to-edit error "close()" to "getStatement()" in comment.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/143#issuecomment-138608383
+1 LGTM
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38710996
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -1334,8 +1335,6 @@ public String getQueryId() throws SQLException {
@Override
protected DrillResultSetImpl execute() throws SQLException{
- DrillConnectionImpl connection = (DrillConnectionImpl) statement.getConnection();
-
connection.getClient().runQuery(QueryType.SQL, this.prepareResult.getSql(),
--- End diff --
I don't know why the code was like that. (Maybe to be symmetric with connection.getDriver(), or maybe from before client was kept?)
Eliminated redundant call to getClient().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38705830
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -850,9 +851,9 @@ public void moveToCurrentRow() throws SQLException {
}
@Override
- public DrillStatementImpl getStatement() {
+ public AvaticaStatement getStatement() {
--- End diff --
Any specific reason to have this method override it's parent implementation ? it's basically doing the same thing
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/143
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38710714
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -87,12 +88,12 @@
boolean hasPendingCancelationNotification;
- DrillResultSetImpl(DrillStatementImpl statement, AvaticaPrepareResult prepareResult,
+ DrillResultSetImpl(AvaticaStatement statement, AvaticaPrepareResult prepareResult,
ResultSetMetaData resultSetMetaData, TimeZone timeZone) {
super(statement, prepareResult, resultSetMetaData, timeZone);
- this.statement = statement;
+ connection = (DrillConnectionImpl) statement.getConnection();
final int batchQueueThrottlingThreshold =
- this.getStatement().getConnection().getClient().getConfig().getInt(
+ connection.getClient().getConfig().getInt(
--- End diff --
Yeah, I don't know why the code was like that.
Reworked a bit to eliminate redundant calls and code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by dsbos <gi...@git.apache.org>.
Github user dsbos commented on a diff in the pull request:
https://github.com/apache/drill/pull/143#discussion_r38708736
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillJdbc41Factory.java ---
@@ -103,9 +103,9 @@ public DrillResultSetImpl newResultSet(AvaticaStatement statement,
TimeZone timeZone) {
final ResultSetMetaData metaData =
newResultSetMetaData(statement, prepareResult.getColumnList());
- return new DrillResultSetImpl( (DrillStatementImpl) statement,
- (DrillPrepareResult) prepareResult,
- metaData, timeZone);
+ return new DrillResultSetImpl(statement,
+ (DrillPrepareResult) prepareResult,
--- End diff --
Simplified.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request: DRILL-3566: Fix: PreparedStatement.executeQuer...
Posted by adeneche <gi...@git.apache.org>.
Github user adeneche commented on the pull request:
https://github.com/apache/drill/pull/143#issuecomment-137597475
@dsbos can you confirm that this patch passes all our "jdbc" tests ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---