You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by vdiravka <gi...@git.apache.org> on 2016/06/13 08:37:10 UTC
[GitHub] drill pull request #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
GitHub user vdiravka opened a pull request:
https://github.com/apache/drill/pull/520
DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Parser will recognize ANSI_SQL identifiers
- added calcite's Lex.MYSQL_ANSI (double quote) supporting for SQL identifiers;
- replaced drill's SqlParser.Config with calcite's one;
- added unit test TestSqlConverter - testAnsiQuotes();
- upgraded getIdentifierQuoteString() for drill's database metadata;
- upgraded unit test testGetIdentifierQuoteString();
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/vdiravka/drill DRILL-3510
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/520.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 #520
----
commit bccb92f4557306ee76f06435211a80cf9464c656
Author: Vitalii Diravka <vi...@gmail.com>
Date: 2016-05-20T20:11:33Z
DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Parser will recognize ANSI_SQL identifiers
- replaced drill's SqlParser.Config with calcite's one;
- added calcite's Lex.MYSQL_ANSI (double quote) supporting for SQL identifiers;
- added unit test TestSqlConverter - testAnsiQuotes();
- upgraded getIdentifierQuoteString() for drill's database metadata;
- upgraded unit test testGetIdentifierQuoteString();
----
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73755627
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -274,6 +274,9 @@
String ENABLE_BULK_LOAD_TABLE_LIST_KEY = "exec.enable_bulk_load_table_list";
BooleanValidator ENABLE_BULK_LOAD_TABLE_LIST = new BooleanValidator(ENABLE_BULK_LOAD_TABLE_LIST_KEY, false);
+ String ANSI_QUOTES_KEY = "parser.ansi_quotes";
--- End diff --
I took that name from the old patch on this jira ticket. Completely agree with a new name.
Done.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r103463608
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -695,6 +698,33 @@ public void runQuery(QueryType type, List<PlanFragment> planFragments, UserResul
}
/**
+ * Get server properties that represent the list of server session options.
+ *
+ * @return server properties for the server session options.
+ */
+ public ServerProperties getOptions() throws RpcException {
--- End diff --
Agree. So I'll use the logic from your PR after it'll be merged into the master.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99866661
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java ---
@@ -204,10 +204,11 @@ public void validate(final OptionValue v, final OptionManager manager) {
* Validator that checks if the given value is included in a list of acceptable values. Case insensitive.
*/
public static class EnumeratedStringValidator extends StringValidator {
- private final Set<String> valuesSet = new HashSet<>();
+ private final Set<String> valuesSet = new LinkedHashSet<>();
public EnumeratedStringValidator(String name, String def, String... values) {
super(name, def);
+ valuesSet.add(def.toLowerCase());
--- End diff --
For example [PARQUET_WRITER_COMPRESSION_TYPE_VALIDATOR](https://github.com/apache/drill/blob/ddcf89548bd33c0cd3e062f1f6d5027eed822372/exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java#L145) uses mentioned approach - `snappy` value is present twice.
Adding of the default value to the set will allow do not duplicate this value in the set of acceptable values.
Using of LinkedHashSet will allow to know that the default value is a first value in the set.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99749402
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java ---
@@ -59,9 +62,11 @@
public static final String USER = "user";
public static final String PASSWORD = "password";
public static final String IMPERSONATION_TARGET = "impersonation_target";
+ public static final String QUOTING_IDENTIFIERS_CHARACTER = "quoting_identifiers_character";
--- End diff --
isn't this a bit too long? could "quoting" be used instead? (also, brackets are two characters... :) )
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99747009
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -695,6 +698,33 @@ public void runQuery(QueryType type, List<PlanFragment> planFragments, UserResul
}
/**
+ * Get server properties that represent the list of server session options.
+ *
+ * @return server properties for the server session options.
+ */
+ public ServerProperties getOptions() throws RpcException {
--- End diff --
I don't think this is the right interface to expose to the user as this is too generic and introduce too much indirect coupling (clients starting dependending on specific options). To be clear, I'm the one to blame here. When we discussed adding metadata methods to the JDBC/ODBC client, one of the things discussed was a server info metadata, to return things like quoting or some other properties (equivalent to JDBC DatabaseMetadata object, or the C++ connector Metadata class: https://github.com/apache/drill/blob/master/contrib/native/client/src/include/drill/drillClient.hpp#L712), but I didn't free some time to add the missing RPC call.
This is probably what we should use here, and hopefully I can probably add it before end of week.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107076868
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java ---
@@ -125,6 +123,15 @@ public Builder setSupportComplexTypes(boolean supportComplexTypes) {
}
public UserSession build() {
+ if (userSession.properties != null && userSession.properties.containsKey(DrillProperties.QUOTING_IDENTIFIERS)) {
--- End diff --
`properties` is already non-null.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107077042
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,18 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call.
+ * Can be used for meta data that can be changed over the session
+ * @return server meta information.
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
unused method?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69997455
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -219,11 +226,26 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ boolean systemOption = false;
+ boolean sessionOption = false;
+ String sql = "select type, bool_val from sys.options where name = 'parser.ansi_quotes'";
+ ResultSet rs = executeSql(sql);
+ while (rs.next()) {
+ if (rs.getString(1).equals("SYSTEM")) {
+ systemOption = rs.getBoolean(2);
+ }
+ if (rs.getString(1).equals("SESSION")) {
+ sessionOption = rs.getBoolean(2);
+ }
+ }
+ if (systemOption || sessionOption) {
--- End diff --
If session option is turned off, but system options is turned on for ansi_quote, do we still return DOULBE_QUOTE?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99823941
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java ---
@@ -161,6 +161,19 @@ public SqlNode parse(String sql) {
SqlParser parser = SqlParser.create(sql, parserConfig);
return parser.parseStmt();
} catch (SqlParseException e) {
+
+ // Attempt to use default back_tick quote character for identifiers, when QUOTING_IDENTIFIERS_CHARACTER
--- End diff --
Yes, it was desirable behaviour:
[DRILL-3510#comment_1](https://issues.apache.org/jira/browse/DRILL-3510?focusedCommentId=14645147&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14645147)
[DRILL-3510#comment_2](https://issues.apache.org/jira/browse/DRILL-3510?focusedCommentId=14943769&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14943769)
Since calcite doesn't support parsing statement with two different characters the only way is this approach. But I agree that it isn't an elegant decision. Therefore I plan to make as you suggested - to return the appropriate error.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106083106
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,17 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call
+ * @return server meta information
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
There's nothing in the JDBC spec which says if this has to be refreshed when user session is changed manually. It seems that MySQL doesn't do a server call each time, but I haven't checked other drivers (not all databases allow to change the quoting identifier in the session).
I would say it might not be necessary as someone who changed manually the quoting know that he/she is dealing with Drill and that he/she changed the the default quoting identifier. But people might have strong arguments in favor of refreshing...
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73758593
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -36,6 +42,7 @@
*/
class DrillDatabaseMetaDataImpl extends AvaticaDatabaseMetaData
--- End diff --
A feature to set on the session option ANSI_QUOTES via jdbc connection string is added in a new commit.
For example: `"jdbc:drill:zk=local;ansi_quotes=true"`
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69990123
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java ---
@@ -274,6 +274,9 @@
String ENABLE_BULK_LOAD_TABLE_LIST_KEY = "exec.enable_bulk_load_table_list";
BooleanValidator ENABLE_BULK_LOAD_TABLE_LIST = new BooleanValidator(ENABLE_BULK_LOAD_TABLE_LIST_KEY, false);
+ String ANSI_QUOTES_KEY = "parser.ansi_quotes";
--- End diff --
this option had better go to PlannerSettings.java, since it's not an option for exec. Also, please follow existing naming convention for new option name. For this case, how about `planner.parser.ansi_quotes`?
---
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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on the issue:
https://github.com/apache/drill/pull/520
[DRILL-4369](https://github.com/apache/drill/commit/13f21e14b4f0832abba8e5632d8fa4e7a6a27635) allows for exchanging version and name information, but these properties do not change after the connection is made.
But identifier quote string option could change, so the only way to know the latest value is to send a RPC request for the current list of server properties, no need to serialize the entire session manager object (if that is [UserSession](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java)).
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107198885
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,18 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call.
+ * Can be used for meta data that can be changed over the session
+ * @return server meta information.
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
Deleted.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106265929
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,17 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call
+ * @return server meta information
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
if you set quoting_identifier in the connection string, it's part of the session, and metadata would return the correct value. But does it need to reflect it if user set it after the connection is established and used?
Also, it seems the JDBC driver for SQLServer always return the default quoting: https://github.com/Microsoft/mssql-jdbc/blob/master/src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java#L836
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107198625
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java ---
@@ -125,6 +123,15 @@ public Builder setSupportComplexTypes(boolean supportComplexTypes) {
}
public UserSession build() {
+ if (userSession.properties != null && userSession.properties.containsKey(DrillProperties.QUOTING_IDENTIFIERS)) {
--- End diff --
Fixed.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69992451
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java ---
@@ -139,6 +151,24 @@ public SqlNode parse(String sql) {
SqlParser parser = SqlParser.create(sql, parserConfig);
return parser.parseStmt();
} catch (SqlParseException e) {
+
+ // Attempt to use default back_tick quote character for identifiers when
--- End diff --
Why would you switch to back_tick quoting, even when option ANSI_QUOTES is on? Why would you want to try with a configuration different from option set by user?
also, sql.contains("`") may not work, if the sql string contains ` as a character in a string literal. I could not see how this block of code will work for such case.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73756251
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java ---
@@ -61,7 +61,8 @@ public static PhysicalPlan getPlan(QueryContext context, String sql, Pointer<Str
context.getNewDefaultSchema(),
context.getDrillOperatorTable(),
(UdfUtilities) context,
- context.getFunctionRegistry());
+ context.getFunctionRegistry(),
+ context.getOptions());
--- End diff --
Corrected.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99624485
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -760,6 +765,24 @@ public void abort(Executor executor) throws SQLException {
}
}
+ @Override
+ public boolean useAnsiQuotedIdentifiers() throws SQLException {
+ boolean systemOption = false;
+ Boolean sessionOption = null;
+ String sql = String.format("select type, bool_val from sys.options where name = '%s'",
+ PlannerSettings.ANSI_QUOTES_KEY);
+ ResultSet rs = executeSql(sql);
--- End diff --
Done.
New RPC request is implemented. RPC response returns `ServerProperties` that represent the list of server session options.
---
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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on the issue:
https://github.com/apache/drill/pull/520
good catch for javax.validation, I don't think I ever tested the driver in a context where the annotations were not present in the classpath
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69997336
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -219,11 +226,26 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ boolean systemOption = false;
+ boolean sessionOption = false;
+ String sql = "select type, bool_val from sys.options where name = 'parser.ansi_quotes'";
--- End diff --
Define a string constant for the option name, and use it across the 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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/520
@sudheeshkatkam Could you please review it?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107077892
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java ---
@@ -262,6 +266,22 @@ public long getParquetRowGroupFilterPushDownThreshold() {
return options.getOption(PARQUET_ROWGROUP_FILTER_PUSHDOWN_PLANNING_THRESHOLD);
}
+ /**
+ * @return Quoting enum for current quoting identifiers character
+ */
+ public Quoting getQuotingIdentifiers() {
--- End diff --
use local var for `options.getOption(QUOTING_IDENTIFIERS)`
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/520
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99824747
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserSession.java ---
@@ -59,9 +62,11 @@
public static final String USER = "user";
public static final String PASSWORD = "password";
public static final String IMPERSONATION_TARGET = "impersonation_target";
+ public static final String QUOTING_IDENTIFIERS_CHARACTER = "quoting_identifiers_character";
--- End diff --
Agree.
I suppose `QUOTING` is not enough. I think `QUOTING_IDENTIFIERS` should be good.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99830247
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java ---
@@ -204,10 +204,11 @@ public void validate(final OptionValue v, final OptionManager manager) {
* Validator that checks if the given value is included in a list of acceptable values. Case insensitive.
*/
public static class EnumeratedStringValidator extends StringValidator {
- private final Set<String> valuesSet = new HashSet<>();
+ private final Set<String> valuesSet = new LinkedHashSet<>();
--- End diff --
To show default value first. For example in the output of exception (TypeValidators.java[#L221](https://github.com/apache/drill/blob/db48298920575cb1c2283e03bdfc7b50e83ae217/exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java#L221)).
Also I'll replace it with guava one.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69997781
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -36,6 +42,7 @@
*/
class DrillDatabaseMetaDataImpl extends AvaticaDatabaseMetaData
--- End diff --
Also, does Drill allow user to specify the ANSI_QUOTE option in JDBC connection string? If not, that means user does not have a way to change ANSI_QOUTE option if they connect through JDBC? If so, then lots of BI tool will not benefit from this patch, if they want to use double quotes in their sql statement.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106453245
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,17 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call
+ * @return server meta information
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
You are right regarding SQLServer JDBC metadata.
Eventually I think for now we can left `getIdentifierQuoteString()` without updating for every call until it will be really necessary to handle direct session changes in DatabaseMetadata.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99748236
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java ---
@@ -204,10 +204,11 @@ public void validate(final OptionValue v, final OptionManager manager) {
* Validator that checks if the given value is included in a list of acceptable values. Case insensitive.
*/
public static class EnumeratedStringValidator extends StringValidator {
- private final Set<String> valuesSet = new HashSet<>();
+ private final Set<String> valuesSet = new LinkedHashSet<>();
public EnumeratedStringValidator(String name, String def, String... values) {
super(name, def);
+ valuesSet.add(def.toLowerCase());
--- End diff --
instead of adding the default to the set, shouldn't we check that def is actually present in values? I think this is how this class has been used until now.
When checking the changes to `PlannerSettings.java`, it's impossible to know which value is actually the default one, whereas it becomes more obvious if the same value is present twice (not perfect though, but at least less error prone. A better change/validator would be to use enums and have something like:
```
class EnumValidator<T extends Enum<T>> extends TypeValidator {
public EnumValidator(String name, Enum<T> default, Class<T> enumClazz) {
...
}
}
```
)
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by sudheeshkatkam <gi...@git.apache.org>.
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69999975
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -1273,5 +1295,11 @@ public boolean generatedKeyAlwaysReturned() throws SQLException {
return super.generatedKeyAlwaysReturned();
}
+ private ResultSet executeSql(String sql) throws SQLException {
--- End diff --
-1
I don't think executing a query is the right way to figure out server properties. Maybe a set of server properties should be returned as part of establishing a connection (identifier quote string being one such property).
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r85241100
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillConnectionImpl.java ---
@@ -760,6 +765,24 @@ public void abort(Executor executor) throws SQLException {
}
}
+ @Override
+ public boolean useAnsiQuotedIdentifiers() throws SQLException {
+ boolean systemOption = false;
+ Boolean sessionOption = null;
+ String sql = String.format("select type, bool_val from sys.options where name = '%s'",
+ PlannerSettings.ANSI_QUOTES_KEY);
+ ResultSet rs = executeSql(sql);
--- End diff --
the statement is not closed, same for the result set, so resources are leaked.
Also, there's an ongoing effort to remove hand-made requests to query metadata and replace those by rpc calls. Shouldn't we introduce a new RPC call to query session state instead? (it also makes things easier for the C++ connector)
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r85241432
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java ---
@@ -241,6 +244,10 @@ public long getInSubqueryThreshold() {
return options.getOption(IN_SUBQUERY_THRESHOLD);
}
+ public boolean isAnsiQuotesEnabled() {
--- End diff --
instead of a boolean, shouldn't we use a enum for the kind of quotes we support, so that we can extend it later?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r103268387
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java ---
@@ -695,6 +698,33 @@ public void runQuery(QueryType type, List<PlanFragment> planFragments, UserResul
}
/**
+ * Get server properties that represent the list of server session options.
+ *
+ * @return server properties for the server session options.
+ */
+ public ServerProperties getOptions() throws RpcException {
--- End diff --
Sorry, it took me longer than a week but PR #764 contains API change for server metadata support with C++ client/JDBC driver support. If approved, it should make things way easier for you as the only change you would need is to update the server metadata to get the quoting information from the session.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106681670
--- Diff: exec/jdbc/pom.xml ---
@@ -100,6 +100,11 @@
<groupId>xalan</groupId>
<artifactId>xalan</artifactId>
</dependency>
+ <dependency>
--- End diff --
aaahh, I thought it was included, but I searched for `@NotNull` instead and didn't find it! So, yes, should totally be included as a dependency!
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r93939395
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java ---
@@ -241,6 +244,10 @@ public long getInSubqueryThreshold() {
return options.getOption(IN_SUBQUERY_THRESHOLD);
}
+ public boolean isAnsiQuotesEnabled() {
--- End diff --
For this reason maybe it makes sense to implement not boolean option ANSI_QUOTES, but some string option, to turn on necessary lexical policy.
I raised this question in jira [DRILL-3510](https://issues.apache.org/jira/browse/DRILL-3510?focusedCommentId=15780561&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15780561)
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by jinfengni <gi...@git.apache.org>.
Github user jinfengni commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r69990725
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlWorker.java ---
@@ -61,7 +61,8 @@ public static PhysicalPlan getPlan(QueryContext context, String sql, Pointer<Str
context.getNewDefaultSchema(),
context.getDrillOperatorTable(),
(UdfUtilities) context,
- context.getFunctionRegistry());
+ context.getFunctionRegistry(),
+ context.getOptions());
--- End diff --
SqlConverter already has pass in PlannerSettings(), why would you pass in another option manager?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73758968
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -219,11 +226,26 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ boolean systemOption = false;
+ boolean sessionOption = false;
+ String sql = "select type, bool_val from sys.options where name = 'parser.ansi_quotes'";
+ ResultSet rs = executeSql(sql);
+ while (rs.next()) {
+ if (rs.getString(1).equals("SYSTEM")) {
+ systemOption = rs.getBoolean(2);
+ }
+ if (rs.getString(1).equals("SESSION")) {
+ sessionOption = rs.getBoolean(2);
+ }
+ }
+ if (systemOption || sessionOption) {
--- End diff --
Missed it. Is corrected in a new commit. Thanks.
---
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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/520
@laurentgo Since you have already reviewed it, could you please take a look at this updated PR. There is only one commit now, because current PR differs very much from the previous one. Anyway I was replied into your previous comments.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73784191
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -1273,5 +1295,11 @@ public boolean generatedKeyAlwaysReturned() throws SQLException {
return super.generatedKeyAlwaysReturned();
}
+ private ResultSet executeSql(String sql) throws SQLException {
--- End diff --
I looked into the implementation of this in mysql. Fixed it in similar manner. But it is still a query.
Is it a good way to send session manager object via RPC?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73757533
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java ---
@@ -139,6 +151,24 @@ public SqlNode parse(String sql) {
SqlParser parser = SqlParser.create(sql, parserConfig);
return parser.parseStmt();
} catch (SqlParseException e) {
+
+ // Attempt to use default back_tick quote character for identifiers when
--- End diff --
One of the requirements was still working identifiers with backticks even while ANSI_QUOTES mode is on.
`sql.contains("`")` condition isn't mandatory here. It was removed in a new commit.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99747702
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/SqlConverter.java ---
@@ -161,6 +161,19 @@ public SqlNode parse(String sql) {
SqlParser parser = SqlParser.create(sql, parserConfig);
return parser.parseStmt();
} catch (SqlParseException e) {
+
+ // Attempt to use default back_tick quote character for identifiers, when QUOTING_IDENTIFIERS_CHARACTER
--- End diff --
is it really a desirable behavior? What's wrong with just returning an error if the quoting doesn't match what has been asked by the user?
A consequence of that change is that every invalid request will be parsed twice even if the issue has nothing to do with quoting.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r85240659
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnection.java ---
@@ -213,4 +213,5 @@ void setNetworkTimeout( Executor executor, int milliseconds )
DrillClient getClient();
+ boolean useAnsiQuotedIdentifiers() throws SQLException;
--- End diff --
it probably be moved to DrillDatabaseMetadataImpl directly (where other metadata requests are made)
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106263502
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -184,6 +184,17 @@ private ServerMeta getServerMeta() throws SQLException {
return serverMeta;
}
+ /**
+ * The same as {@link DrillDatabaseMetaDataImpl#getServerMeta()} but updates server metadata for the every call
+ * @return server meta information
+ * @throws SQLException for error when getting server meta
+ */
+ private ServerMeta getUpdatedServerMeta() throws SQLException {
--- End diff --
SQL Server, for example, updates metadata in some cases
https://www.mssqltips.com/sqlservertip/1415/determining-set-options-for-a-current-session-in-sql-server/
Quoting_identifiers can be changes as system option. In this case any user can get the right and actual quoting_identifiers method from server_metadata.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r73758663
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -219,11 +226,26 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ boolean systemOption = false;
+ boolean sessionOption = false;
+ String sql = "select type, bool_val from sys.options where name = 'parser.ansi_quotes'";
--- End diff --
Done.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99623110
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnection.java ---
@@ -213,4 +213,5 @@ void setNetworkTimeout( Executor executor, int milliseconds )
DrillClient getClient();
+ boolean useAnsiQuotedIdentifiers() throws SQLException;
--- End diff --
Done.
This is the similar approach like in mySQL - setting quoting identifier character during the connection time.
Since a new RPC request is implemented, it is no longer necessary.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106658488
--- Diff: exec/jdbc-all/pom.xml ---
@@ -403,7 +402,6 @@
<exclude>**/*.SF</exclude>
<exclude>**/*.RSA</exclude>
<exclude>**/*.DSA</exclude>
- <exclude>javax/**</exclude>
--- End diff --
This ` <exclude>javax/**</exclude>` exludes validation-api as well. Therefore it should be deleted.
To avoid including unnecessary libraries I decided to add:
` <exclude>javax/activation/**</exclude>`
` <exclude>javax/annotation-api/**</exclude>`
` <exclude>javax/inject/**</exclude>`
` <exclude>javax/servlet-api/**</exclude>`
` <exclude>javax/json/**</exclude>`
` <exclude>javax/ws/**</exclude>`
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99749138
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -231,11 +274,16 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ Property property = getServerProperty(PlannerSettings.QUOTING_IDENTIFIERS_CHARACTER_KEY);
+ for (Quoting value : Quoting.values()) {
+ if (value.string.equals(property.getValue())) {
+ return value.string;
+ }
+ }
+ throw new SQLException("Unknown quoting identifier character " + property.getValue());
--- End diff --
we probably need a fallback (see my pr #613 )
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106480140
--- Diff: exec/jdbc-all/pom.xml ---
@@ -403,7 +402,6 @@
<exclude>**/*.SF</exclude>
<exclude>**/*.RSA</exclude>
<exclude>**/*.DSA</exclude>
- <exclude>javax/**</exclude>
--- End diff --
maybe just javax.validation?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r107198532
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/PlannerSettings.java ---
@@ -262,6 +266,22 @@ public long getParquetRowGroupFilterPushDownThreshold() {
return options.getOption(PARQUET_ROWGROUP_FILTER_PUSHDOWN_PLANNING_THRESHOLD);
}
+ /**
+ * @return Quoting enum for current quoting identifiers character
+ */
+ public Quoting getQuotingIdentifiers() {
--- End diff --
Done. Thanks.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106479965
--- Diff: exec/jdbc/pom.xml ---
@@ -100,6 +100,11 @@
<groupId>xalan</groupId>
<artifactId>xalan</artifactId>
</dependency>
+ <dependency>
--- End diff --
Might not be needed. I don't think Drill depends directly on it (but only transitively through Avatica)
---
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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/520
@sudheeshkatkam The branch is rebased to the latest master version. Please keep the both commits for the master version.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r103672254
--- Diff: exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillDatabaseMetaDataImpl.java ---
@@ -231,11 +274,16 @@ public boolean storesMixedCaseQuotedIdentifiers() throws SQLException {
return super.storesMixedCaseQuotedIdentifiers();
}
- // TODO(DRILL-3510): Update when Drill accepts standard SQL's double quote.
@Override
public String getIdentifierQuoteString() throws SQLException {
throwIfClosed();
- return "`";
+ Property property = getServerProperty(PlannerSettings.QUOTING_IDENTIFIERS_CHARACTER_KEY);
+ for (Quoting value : Quoting.values()) {
+ if (value.string.equals(property.getValue())) {
+ return value.string;
+ }
+ }
+ throw new SQLException("Unknown quoting identifier character " + property.getValue());
--- End diff --
Agree. I will add it after merging your's PRs (#613 and #764) to use logic from there.
---
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 issue #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's SQL Par...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on the issue:
https://github.com/apache/drill/pull/520
@laurentgo The branch is updated to master drill-1.11.0-SNAPSHOT.
Quoting_identifiers metadata is left without handling direct session changes.
Also I found one issue, when tried to connect to drillbit by SQuirrel via jdbc:
`java.lang.NoClassDefFoundError: javax/validation/constraints/NotNull
at org.apache.drill.jdbc.impl.DrillMetaImpl.drillFieldMetaData(DrillMetaImpl.java:263)`.
This annotation was added in DRILL-5301.
I decided to add a new dependency for jdbc module in the context of this PR.
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106681714
--- Diff: exec/jdbc-all/pom.xml ---
@@ -403,7 +402,6 @@
<exclude>**/*.SF</exclude>
<exclude>**/*.RSA</exclude>
<exclude>**/*.DSA</exclude>
- <exclude>javax/**</exclude>
--- End diff --
sounds good!
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r99747062
--- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/TypeValidators.java ---
@@ -204,10 +204,11 @@ public void validate(final OptionValue v, final OptionManager manager) {
* Validator that checks if the given value is included in a list of acceptable values. Case insensitive.
*/
public static class EnumeratedStringValidator extends StringValidator {
- private final Set<String> valuesSet = new HashSet<>();
+ private final Set<String> valuesSet = new LinkedHashSet<>();
--- End diff --
is ordering important?
---
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 #520: DRILL-3510: Add ANSI_QUOTES option so that Drill's ...
Posted by vdiravka <gi...@git.apache.org>.
Github user vdiravka commented on a diff in the pull request:
https://github.com/apache/drill/pull/520#discussion_r106654176
--- Diff: exec/jdbc/pom.xml ---
@@ -100,6 +100,11 @@
<groupId>xalan</groupId>
<artifactId>xalan</artifactId>
</dependency>
+ <dependency>
--- End diff --
Drill uses NotNull annotations in [DrillMetaImpl.java](https://github.com/apache/drill/blob/master/exec/jdbc/src/main/java/org/apache/drill/jdbc/impl/DrillMetaImpl.java#L263).
`validation-api` is present only in the dependency of the `jersey-server-2.8.jar`, which is excluded in the jdbc, as it should be.
---
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.
---