You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/08/29 20:09:33 UTC
[GitHub] [hive] kishendas opened a new pull request #1443: Implement jdbc methods invoked by Calcite
kishendas opened a new pull request #1443:
URL: https://github.com/apache/hive/pull/1443
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Implement below JDBC methods
nullsAreSortedAtEnd
nullsAreSortedAtStart
nullsAreSortedLow
nullsAreSortedHigh
storesLowerCaseIdentifiers
storesLowerCaseQuotedIdentifiers
storesMixedCaseIdentifiers
storesMixedCaseQuotedIdentifiers
storesUpperCaseIdentifiers
storesUpperCaseQuotedIdentifiers
supportsMixedCaseIdentifiers
supportsMixedCaseQuotedIdentifiers
### Why are the changes needed?
Calcite may rely on the following JDBC methods to generate SQL queries for Hive JDBC storage handler, which in the case of Hive itself, return a Method not supported exception. We should implement such methods, as listed below.
nullsAreSortedAtEnd
nullsAreSortedAtStart
nullsAreSortedLow
nullsAreSortedHigh
storesLowerCaseIdentifiers
storesLowerCaseQuotedIdentifiers
storesMixedCaseIdentifiers
storesMixedCaseQuotedIdentifiers
storesUpperCaseIdentifiers
storesUpperCaseQuotedIdentifiers
supportsMixedCaseIdentifiers
supportsMixedCaseQuotedIdentifiers
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
This change relies on existing tests.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r483268638
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,18 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ /**
+ * This returns Hive configuration for HIVE_DEFAULT_NULLS_LAST.
+ *
+ * @param hiveConfs
+ * @return
+ */
+ public static boolean getHiveDefaultNullsLast(Map<String, String> hiveConfs) {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
+ if ((hiveConfs != null) && (hiveConfs.get(JdbcConnectionParams.HIVE_DEFAULT_NULLS_LAST_KEY) != null)) {
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r482524319
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -855,19 +860,19 @@ public boolean nullPlusNonNullIsNull() throws SQLException {
}
public boolean nullsAreSortedAtEnd() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedAtStart() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedHigh() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return !getHiveDefaultNullsLast(connection.getConnParams().getHiveConfs());
Review comment:
Shouldn't this be the opposite, i.e., isn't `hive.default.nulls.last` same as `nullsAreSortedHigh`?
```
Whether to set NULLS LAST as the default null ordering for ASC order.
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] thejasmn commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
thejasmn commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r479722078
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java
##########
@@ -291,7 +291,7 @@ public HiveConnection(String uri, Properties info) throws SQLException {
if (isEmbeddedMode) {
client = EmbeddedCLIServicePortal.get(connParams.getHiveConfs());
- connParams.getHiveConfs().clear();
+ connParams.getHiveConfs().clear();
Review comment:
extra space, pls fix indent
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
HIVE_DEFAULT_NULLS_LAST could be overridden on the HiveServer2 using hive-site.xml settings. We need to get a value of this from HS2.
I am not sure if we get the current settings from HS2 (i need to check).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] thejasmn commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
thejasmn commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r479722648
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -855,19 +860,19 @@ public boolean nullPlusNonNullIsNull() throws SQLException {
}
public boolean nullsAreSortedAtEnd() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedAtStart() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedHigh() throws SQLException {
Review comment:
An unit test would be useful for nullsAreSortedHigh and nullsAreSortedLow
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r483268575
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java
##########
@@ -866,6 +874,13 @@ private void openSession() throws SQLException {
try {
TOpenSessionResp openResp = client.OpenSession(openReq);
+ // Override HS2 server HiveConf in Connection parameter HiveConf
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r483168530
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,18 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ /**
+ * This returns Hive configuration for HIVE_DEFAULT_NULLS_LAST.
+ *
+ * @param hiveConfs
+ * @return
+ */
+ public static boolean getHiveDefaultNullsLast(Map<String, String> hiveConfs) {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
+ if ((hiveConfs != null) && (hiveConfs.get(JdbcConnectionParams.HIVE_DEFAULT_NULLS_LAST_KEY) != null)) {
Review comment:
I think there can be JDBC tests where hiveConf can come as null. I can change this and see how many tests fail.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r484047791
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -326,10 +327,12 @@ public TOpenSessionResp OpenSession(TOpenSessionReq req) throws TException {
final int fetchSize = hiveConf.getIntVar(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_RESULTSET_DEFAULT_FETCH_SIZE);
+ Map<String, String> map = new HashMap<>();
Review comment:
Updated the existing test case TestThriftCliServiceWithInfoMessage.java for this. Please take a look, when you get time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on pull request #1443:
URL: https://github.com/apache/hive/pull/1443#issuecomment-687905937
@jcamachor @thejasmn Got a green run and all the comments have been addressed. Please check, when you folks get time.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r481415157
##########
File path: itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestHiveDatabaseMetaData.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hive.jdbc;
+
+import org.apache.hive.jdbc.HiveConnection;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hive.jdbc.Utils;
+import org.apache.hive.jdbc.Utils.JdbcConnectionParams;
+
+
+import java.util.HashMap;
+import java.util.Properties;
+import java.util.Map;
+import java.sql.SQLException;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+/**
+ * TestHiveDatabaseMetaData.
+ *
+ */
+public class TestHiveDatabaseMetaData {
+
+ private static final Map<String, String> map = new HashMap<>();
+ private HiveDatabaseMetaData hiveDatabaseMetaData;
+
+ @Before
+ public void setup() throws Exception {
+ map.put(Utils.HIVE_CONF_PREFIX + ConfVars.HIVE_DEFAULT_NULLS_LAST, "false");
Review comment:
Done
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] thejasmn commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
thejasmn commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r481295792
##########
File path: itests/hive-unit/src/test/java/org/apache/hive/jdbc/TestHiveDatabaseMetaData.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hive.jdbc;
+
+import org.apache.hive.jdbc.HiveConnection;
+import org.apache.hadoop.hive.conf.HiveConf.ConfVars;
+import org.apache.hive.jdbc.Utils;
+import org.apache.hive.jdbc.Utils.JdbcConnectionParams;
+
+
+import java.util.HashMap;
+import java.util.Properties;
+import java.util.Map;
+import java.sql.SQLException;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+/**
+ * TestHiveDatabaseMetaData.
+ *
+ */
+public class TestHiveDatabaseMetaData {
+
+ private static final Map<String, String> map = new HashMap<>();
+ private HiveDatabaseMetaData hiveDatabaseMetaData;
+
+ @Before
+ public void setup() throws Exception {
+ map.put(Utils.HIVE_CONF_PREFIX + ConfVars.HIVE_DEFAULT_NULLS_LAST, "false");
Review comment:
Can you also please test for value of true ? That way it checks that method return value is changing based on the config param value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r483268877
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -326,10 +327,12 @@ public TOpenSessionResp OpenSession(TOpenSessionReq req) throws TException {
final int fetchSize = hiveConf.getIntVar(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_RESULTSET_DEFAULT_FETCH_SIZE);
+ Map<String, String> map = new HashMap<>();
Review comment:
Sure, I will look into this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor merged pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
jcamachor merged pull request #1443:
URL: https://github.com/apache/hive/pull/1443
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r481291877
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
I think @thejasmn concern is valid. If user specifies the property value in the connection parameters, then we will retrieve it here. However, if it is not present, we probably need to establish a connection to HS2 to retrieve the parameter value, since it may not be necessarily the default?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r480433028
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
In Beeline we pick the Hive config and prefix it with hiveconf: and put it in the JDBCParameters.hiveConf. So, I have added the prefix, while looking up for the relevant key in the map. Not sure if there is any other place, where we send the hive conf differently.
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -855,19 +860,19 @@ public boolean nullPlusNonNullIsNull() throws SQLException {
}
public boolean nullsAreSortedAtEnd() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedAtStart() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedHigh() throws SQLException {
Review comment:
Added
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] thejasmn commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
thejasmn commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r483153493
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveConnection.java
##########
@@ -866,6 +874,13 @@ private void openSession() throws SQLException {
try {
TOpenSessionResp openResp = client.OpenSession(openReq);
+ // Override HS2 server HiveConf in Connection parameter HiveConf
Review comment:
1. The setting in client side should override the server settings. ie, server setting should be used if no such value is set in the client side
2. Can we make this generic ? Ie, loop through all the server settings and set it in hiveConfs only if there is no value set currently ?
##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftCLIService.java
##########
@@ -326,10 +327,12 @@ public TOpenSessionResp OpenSession(TOpenSessionReq req) throws TException {
final int fetchSize = hiveConf.getIntVar(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_RESULTSET_DEFAULT_FETCH_SIZE);
+ Map<String, String> map = new HashMap<>();
Review comment:
Can we add a test to verify we are getting the config value from server ? Either in TestHiveDatabaseMetaData or a different test ?
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,18 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ /**
+ * This returns Hive configuration for HIVE_DEFAULT_NULLS_LAST.
+ *
+ * @param hiveConfs
+ * @return
+ */
+ public static boolean getHiveDefaultNullsLast(Map<String, String> hiveConfs) {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
+ if ((hiveConfs != null) && (hiveConfs.get(JdbcConnectionParams.HIVE_DEFAULT_NULLS_LAST_KEY) != null)) {
Review comment:
This if statement should never return false. Something is wrong if it does, we should throw an error in that case (otherwise, we might run into some harder to debug issues).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r481422408
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
@jcamachor @thejasmn Looks like we already open the session with HS2 in HiveConnection, but when I see the OpenSession implementation in ThriftCLIService, looks like we only send one HiveConf configuration :
resp.setConfiguration(Collections
.singletonMap(HiveConf.ConfVars.HIVE_SERVER2_THRIFT_RESULTSET_DEFAULT_FETCH_SIZE.varname,
Integer.toString(fetchSize))); Should I add HIVE_DEFAULT_NULLS_LAST here or we need to expose a new method ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r482558418
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
New code makes sense to me. @thejasmn could you take a quick look in case I am missing anything?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r482333075
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
@jcamachor @thejasmn Please check if the changes look good.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kishendas commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
kishendas commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r482602169
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -855,19 +860,19 @@ public boolean nullPlusNonNullIsNull() throws SQLException {
}
public boolean nullsAreSortedAtEnd() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedAtStart() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return false;
}
public boolean nullsAreSortedHigh() throws SQLException {
- throw new SQLFeatureNotSupportedException("Method not supported");
+ return !getHiveDefaultNullsLast(connection.getConnParams().getHiveConfs());
Review comment:
Right, fixed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on a change in pull request #1443: Implement jdbc methods invoked by Calcite
Posted by GitBox <gi...@apache.org>.
jcamachor commented on a change in pull request #1443:
URL: https://github.com/apache/hive/pull/1443#discussion_r481464936
##########
File path: jdbc/src/java/org/apache/hive/jdbc/HiveDatabaseMetaData.java
##########
@@ -1227,4 +1232,13 @@ private TGetInfoResp getServerInfo(TGetInfoType type) throws SQLException {
Utils.verifySuccess(resp.getStatus());
return resp;
}
+
+ private boolean getHiveDefaultNullsLast() {
+ boolean response = ConfVars.HIVE_DEFAULT_NULLS_LAST.defaultBoolVal;
Review comment:
@kishendas , I think it makes sense to return that property value too when the session is open since the mechanism is already in place and adding an additional property value should not create much overhead.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org