You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vvysotskyi <gi...@git.apache.org> on 2018/03/30 16:46:57 UTC
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
GitHub user vvysotskyi opened a pull request:
https://github.com/apache/drill/pull/1198
DRILL-6294: Changes to support Calcite 1.16.0
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vvysotskyi/drill DRILL-6294
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/1198.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 #1198
----
commit a79d6586033b618c95462368520237aab84f47bf
Author: Volodymyr Vysotskyi <vv...@...>
Date: 2018-02-07T14:24:50Z
DRILL-6294: Changes to support Calcite 1.16.0
commit 48880eb80d60a15e4ffdfcd6c729bfc75cf5d2da
Author: Volodymyr Vysotskyi <vv...@...>
Date: 2018-03-11T10:11:45Z
DRILL-6294: Remove deprecated API usage
----
---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/1198#discussion_r178395314
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -1142,16 +1142,8 @@ public void moveToCurrentRow() throws SQLException {
}
@Override
- public AvaticaStatement getStatement() {
- try {
- throwIfClosed();
- } catch (AlreadyClosedSqlException e) {
- // Can't throw any SQLException because AvaticaConnection's
- // getStatement() is missing "throws SQLException".
- throw new RuntimeException(e.getMessage(), e);
- } catch (SQLException e) {
- throw new RuntimeException(e.getMessage(), e);
- }
+ public AvaticaStatement getStatement() throws SQLException {
+ throwIfClosed();
--- End diff --
Since you are touching this file. You might want to remove not needed Exceptions for throwIfClosed() method that are derives of SqlException.
---
[GitHub] drill issue #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by vvysotskyi <gi...@git.apache.org>.
Github user vvysotskyi commented on the issue:
https://github.com/apache/drill/pull/1198
@chunhui-shi, I have addressed all CR comments, could you please take a look again?
---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/1198#discussion_r178364263
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java ---
@@ -218,7 +218,8 @@ private void reduceAggs(
RelOptUtil.createProject(
newAggRel,
projList,
- oldAggRel.getRowType().getFieldNames());
+ oldAggRel.getRowType().getFieldNames(),
+ DrillRelFactories.LOGICAL_BUILDER);
--- End diff --
Could you explain why we are using DrillRelFactories.LOGICAL_BUILDER but not relBuilderFactory that was used in line 211? And could you point me to this 4 param createProject method with Factory as the last param?
---
[GitHub] drill issue #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by chunhui-shi <gi...@git.apache.org>.
Github user chunhui-shi commented on the issue:
https://github.com/apache/drill/pull/1198
+1. Thank you for making the fix.
---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/1198
---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by vvysotskyi <gi...@git.apache.org>.
Github user vvysotskyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/1198#discussion_r178371341
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java ---
@@ -218,7 +218,8 @@ private void reduceAggs(
RelOptUtil.createProject(
newAggRel,
projList,
- oldAggRel.getRowType().getFieldNames());
+ oldAggRel.getRowType().getFieldNames(),
+ DrillRelFactories.LOGICAL_BUILDER);
--- End diff --
In the second commit it was fixed and used relBuilderFactory to create builder and project.
---
[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by vvysotskyi <gi...@git.apache.org>.
Github user vvysotskyi commented on a diff in the pull request:
https://github.com/apache/drill/pull/1198#discussion_r178404654
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillResultSetImpl.java ---
@@ -1142,16 +1142,8 @@ public void moveToCurrentRow() throws SQLException {
}
@Override
- public AvaticaStatement getStatement() {
- try {
- throwIfClosed();
- } catch (AlreadyClosedSqlException e) {
- // Can't throw any SQLException because AvaticaConnection's
- // getStatement() is missing "throws SQLException".
- throw new RuntimeException(e.getMessage(), e);
- } catch (SQLException e) {
- throw new RuntimeException(e.getMessage(), e);
- }
+ public AvaticaStatement getStatement() throws SQLException {
+ throwIfClosed();
--- End diff --
Thanks, it looks clearer now.
---
[GitHub] drill issue #1198: DRILL-6294: Changes to support Calcite 1.16.0
Posted by vvysotskyi <gi...@git.apache.org>.
Github user vvysotskyi commented on the issue:
https://github.com/apache/drill/pull/1198
@amansinha100, @chunhui-shi could you please take a look?
---