[GitHub] drill pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0

2018-03-30 Thread vvysotskyi 
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 
Date:   2018-02-07T14:24:50Z

DRILL-6294: Changes to support Calcite 1.16.0

commit 48880eb80d60a15e4ffdfcd6c729bfc75cf5d2da
Author: Volodymyr Vysotskyi 
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

2018-03-30 Thread chunhui-shi 
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 pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0

2018-03-30 Thread vvysotskyi 
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

2018-03-30 Thread chunhui-shi 
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 pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0

2018-03-30 Thread vvysotskyi 
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 pull request #1198: DRILL-6294: Changes to support Calcite 1.16.0

2018-04-13 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/drill/pull/1198


---