[GitHub] [drill] paul-rogers commented on a change in pull request #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query
paul-rogers commented on a change in pull request #2052: DRILL-7603 and
DRILL-7604: Add schema, options to REST query
URL: https://github.com/apache/drill/pull/2052#discussion_r407158975
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/server/options/OptionValue.java
##
@@ -146,6 +164,40 @@ public static OptionValue create(AccessibleScopes type,
String name, Object val,
throw new IllegalArgumentException(String.format("Unsupported type %s",
val.getClass()));
}
+ private static OptionValue fromString(AccessibleScopes type, String name,
+ String val, OptionScope scope, Kind kind) {
+val = val.trim();
Review comment:
No, we don't. However, added an argument check to enforce non-null.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query
paul-rogers commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query URL: https://github.com/apache/drill/pull/2052#issuecomment-612576587 @arina-ielchiieva, thanks much for the review. As far as I could tell, the only UI changes in the original PR were for the default schema. It would take a bit of effort to create a UI for a list of options. In fact, it might be better to extend the back-end to allow the user to post a series of queries in one go, with all but the last being trivial. This approach would be easier for humans because there is no need to figure out how a UI maps to a command: ``` USE foo; ALTER SESSION SET `bar` = 'mumble'; SELECT ... FROM something ... ``` For the REST API, it is probably easier to include these extras in the REST request. A clever client can accumulate changes and include them with each query request. The REST API can allow additional features that the UI ignores. In the case of options, the UI won't post any and the back-end will skip the options step. What do you think? Finally, thanks for pointing out DRILL-7655. How did that get filed if the original PR never made it into master? Anyway, I agree it should be fixed and I'll look into that for this PR. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query
arina-ielchiieva commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query URL: https://github.com/apache/drill/pull/2052#issuecomment-612607875 @paul-rogers sounds good. Yes, original PR did not contains Web UI functionality but I was writing there that it might be nice to have. But I am ok if we won't do right now. Regarding DRILL-7655, original PR made into Drill actually. There were two of them and one with the schema actually got it (https://github.com/apache/drill/commit/a8c9a0f75477cb49284e337d5ebcda51318f4380). 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437:
Storage Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407192664
##
File path: contrib/storage-http/pom.xml
##
@@ -0,0 +1,97 @@
+
+
+http://maven.apache.org/POM/4.0.0";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+
+drill-contrib-parent
+org.apache.drill.contrib
+1.18.0-SNAPSHOT
+
+
+ drill-storage-http
+ contrib/http-storage-plugin
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ ${project.version}
+
+
+ com.squareup.okhttp3
+ okhttp
+ 4.2.2
Review comment:
Please use latest version
https://mvnrepository.com/artifact/com.squareup.okhttp3/okhttp
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437:
Storage Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407192812
##
File path: contrib/storage-http/pom.xml
##
@@ -0,0 +1,97 @@
+
+
+http://maven.apache.org/POM/4.0.0";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+
+drill-contrib-parent
+org.apache.drill.contrib
+1.18.0-SNAPSHOT
+
+
+ drill-storage-http
+ contrib/http-storage-plugin
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ ${project.version}
+
+
+ com.squareup.okhttp3
+ okhttp
+ 4.2.2
+
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ tests
+ ${project.version}
+ test
+
+
+
+ org.apache.drill
+ drill-common
+ tests
+ ${project.version}
+ test
+
+
+
+ com.squareup.okhttp3
+ mockwebserver
+ 4.2.2
Review comment:
Please use latest version
https://mvnrepository.com/artifact/com.squareup.okhttp3/mockwebserver.
Actully use can make one property for both since versions are synced:
```
4.5.0
```
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437:
Storage Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407192877
##
File path:
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java
##
@@ -0,0 +1,148 @@
+/*
+ * 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.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.drill.common.PlanStringBuilder;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+import java.util.Objects;
+
+public class HttpAPIConfig {
+ private static final Logger logger =
LoggerFactory.getLogger(HttpAPIConfig.class);
+
+ private final String url;
+
+ private final String method;
+
+ private final Map headers;
+
+ private final String authType;
+
+ private final String userName;
+
+ private final String password;
+
+ private final String postBody;
+
+ public HttpAPIConfig(@JsonProperty("url") String url,
+ @JsonProperty("method") String method,
+ @JsonProperty("headers") Map headers,
+ @JsonProperty("authType") String authType,
+ @JsonProperty("userName") String userName,
+ @JsonProperty("password") String password,
+ @JsonProperty("postBody") String postBody) {
+
+this.headers = headers;
+this.method = Strings.isNullOrEmpty(method)
+? "GET" : method.trim().toUpperCase();
+
+// Get the request method. Only accept GET and POST requests. Anything
else will default to GET.
+switch (this.method) {
+ case "GET":
Review comment:
Maybe create enum?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437:
Storage Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407193553
##
File path: contrib/storage-http/src/test/resources/data/sample_data.json
##
@@ -0,0 +1,1000 @@
+[{"id":1,"first_name":"Eba","last_name":"Le
Sarr","email":"elesa...@springer.com","gender":"Female","ip_address":"249.113.19.117","random_number":92.3},
Review comment:
We don't need such big file for tests, please make it as small as possible.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437:
Storage Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407193467
##
File path:
contrib/storage-kudu/src/main/java/org/apache/drill/exec/store/kudu/KuduStoragePlugin.java
##
@@ -35,15 +35,20 @@
private final KuduSchemaFactory schemaFactory;
private final KuduClient client;
-
public KuduStoragePlugin(KuduStoragePluginConfig configuration,
DrillbitContext context, String name)
- throws IOException {
+throws IOException {
super(context, name);
this.schemaFactory = new KuduSchemaFactory(this, name);
this.engineConfig = configuration;
this.client = new
KuduClient.KuduClientBuilder(configuration.getMasterAddresses()).build();
}
+
+ @Override
+ public void start() throws IOException {
+
+ }
Review comment:
Please remove as @paul-rogers suggested.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
arina-ielchiieva commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API URL: https://github.com/apache/drill/pull/1892#discussion_r407193393 ## File path: contrib/storage-http/src/test/resources/logback-test.xml.bak ## @@ -0,0 +1,69 @@ + Review comment: Please remove logback-test.xml - all Drill packages use common one from `drill-common` module. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407217196
##
File path:
contrib/storage-kudu/src/main/java/org/apache/drill/exec/store/kudu/KuduStoragePlugin.java
##
@@ -35,15 +35,20 @@
private final KuduSchemaFactory schemaFactory;
private final KuduClient client;
-
public KuduStoragePlugin(KuduStoragePluginConfig configuration,
DrillbitContext context, String name)
- throws IOException {
+throws IOException {
super(context, name);
this.schemaFactory = new KuduSchemaFactory(this, name);
this.engineConfig = configuration;
this.client = new
KuduClient.KuduClientBuilder(configuration.getMasterAddresses()).build();
}
+
+ @Override
+ public void start() throws IOException {
+
+ }
Review comment:
Fixed.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API URL: https://github.com/apache/drill/pull/1892#discussion_r407217273 ## File path: contrib/storage-http/src/test/resources/logback-test.xml.bak ## @@ -0,0 +1,69 @@ + Review comment: Removed 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407219777
##
File path: contrib/storage-http/pom.xml
##
@@ -0,0 +1,97 @@
+
+
+http://maven.apache.org/POM/4.0.0";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+
+drill-contrib-parent
+org.apache.drill.contrib
+1.18.0-SNAPSHOT
+
+
+ drill-storage-http
+ contrib/http-storage-plugin
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ ${project.version}
+
+
+ com.squareup.okhttp3
+ okhttp
+ 4.2.2
+
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ tests
+ ${project.version}
+ test
+
+
+
+ org.apache.drill
+ drill-common
+ tests
+ ${project.version}
+ test
+
+
+
+ com.squareup.okhttp3
+ mockwebserver
+ 4.2.2
Review comment:
Fixed
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407219785
##
File path: contrib/storage-http/pom.xml
##
@@ -0,0 +1,97 @@
+
+
+http://maven.apache.org/POM/4.0.0";
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance";
+ xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd";>
+ 4.0.0
+
+
+drill-contrib-parent
+org.apache.drill.contrib
+1.18.0-SNAPSHOT
+
+
+ drill-storage-http
+ contrib/http-storage-plugin
+
+
+
+ org.apache.drill.exec
+ drill-java-exec
+ ${project.version}
+
+
+ com.squareup.okhttp3
+ okhttp
+ 4.2.2
Review comment:
Fixed
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407221404
##
File path: contrib/storage-http/src/test/resources/data/sample_data.json
##
@@ -0,0 +1,1000 @@
+[{"id":1,"first_name":"Eba","last_name":"Le
Sarr","email":"elesa...@springer.com","gender":"Female","ip_address":"249.113.19.117","random_number":92.3},
Review comment:
I removed the file and the one unit test that used 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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407226509
##
File path:
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java
##
@@ -0,0 +1,148 @@
+/*
+ * 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.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.drill.common.PlanStringBuilder;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+import java.util.Objects;
+
+public class HttpAPIConfig {
+ private static final Logger logger =
LoggerFactory.getLogger(HttpAPIConfig.class);
+
+ private final String url;
+
+ private final String method;
+
+ private final Map headers;
+
+ private final String authType;
+
+ private final String userName;
+
+ private final String password;
+
+ private final String postBody;
+
+ public HttpAPIConfig(@JsonProperty("url") String url,
+ @JsonProperty("method") String method,
+ @JsonProperty("headers") Map headers,
+ @JsonProperty("authType") String authType,
+ @JsonProperty("userName") String userName,
+ @JsonProperty("password") String password,
+ @JsonProperty("postBody") String postBody) {
+
+this.headers = headers;
+this.method = Strings.isNullOrEmpty(method)
+? "GET" : method.trim().toUpperCase();
+
+// Get the request method. Only accept GET and POST requests. Anything
else will default to GET.
+switch (this.method) {
+ case "GET":
Review comment:
Converted to enum.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on issue #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on issue #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API URL: https://github.com/apache/drill/pull/1892#issuecomment-612647384 @arina-ielchiieva I addressed your review comments. Could you please take a final look and let me know if we are good to go? Thanks! 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: us...@infra.apache.org With regards, Apache Git Services
[jira] [Created] (DRILL-7697) Revise query editor in profile page of web UI
Paul Rogers created DRILL-7697: -- Summary: Revise query editor in profile page of web UI Key: DRILL-7697 URL: https://issues.apache.org/jira/browse/DRILL-7697 Project: Apache Drill Issue Type: Improvement Affects Versions: 1.17.0 Reporter: Paul Rogers Drill has two separate query editors: * The one displayed from the Query tab * The one displayed from the Edit Query tab within Profiles The two editors do basically the same thing, but have evolved as copies that have diverged. * The Query tab editor places the three query types above the query text box, while the Profiles version puts the same control below the query text box. * Similarly the Query tab editor puts the Ctrl+Enter hint above the text box, Profiles puts it below. A first request is to unify the two editors. In particular, move the code to a common template file included in both places. Second, the Profiles editor is a bit redundant. * Displays a "Cancel Query" button even if the query is completed. Hide this button for completed queries. (Since there is a race condition, hide it for queries completed at the time the page was created.) * No need to ask the user for the query type. The profile should include the type and the type should be a fixed field in Profiles. * Similarly, the limit and (in Drill 1.18) the Default Schema should also be recorded in the query plan and fixed. Finally, since system/session options can affect a query, and are part of the query plan, show those in the query as well so it can be rerun in the same environment in which it originally ran. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [drill] paul-rogers commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query
paul-rogers commented on issue #2052: DRILL-7603 and DRILL-7604: Add schema, options to REST query URL: https://github.com/apache/drill/pull/2052#issuecomment-612655336 Updated to add the default schema to the Profiles query editor. Filed DRILL-7697 for additional improvements, such as storing the default schema in the profile so we can rerun the query without the user having to specify the schema manually. Noticed that the two editors had drifted apart visually, so tried to unify them a bit. DRILL-7697 asks for the proper solution: a single template file included in both places. Fixed the error you mentioned. Added a simple test program to allow testing the UI from the debugger without having to wait for a full build. 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: us...@infra.apache.org With regards, Apache Git Services
[jira] [Resolved] (DRILL-7685) Case statement marking column as required in parquet metadata
[ https://issues.apache.org/jira/browse/DRILL-7685?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Paul Rogers resolved DRILL-7685. Resolution: Cannot Reproduce Tested in Drill 1.18 (snapshot) and found that the provided query works fine. Suggested the user try the newer Drill version. If you still have a problem please reopen this bug and provide another example so we can locate and fix the issue, if it still exists in the latest code. > Case statement marking column as required in parquet metadata > - > > Key: DRILL-7685 > URL: https://issues.apache.org/jira/browse/DRILL-7685 > Project: Apache Drill > Issue Type: Improvement > Components: Storage - Parquet >Affects Versions: 1.16.0 >Reporter: Nitin Pawar >Assignee: Paul Rogers >Priority: Minor > > We use apache drill for multi step processing. > In one of the steps we have query as below > ~create table dfs.tmp.`/t2` as select employee_id, case when department_id is > not null then 1 else 2 end as case_output from cp.`employee.json`;~ > This provides output as > employee_id: OPTIONAL INT64 R:0 D:1 > case_output: REQUIRED INT32 R:0 D:0 > If we remove the end statement from case it does mark the column as optional. > > We feed this output to covariance function and because of this we get an > error like below > Error: Missing function implementation: [covariance(BIGINT-OPTIONAL, > INT-REQUIRED)]. Full expression: --UNKNOWN EXPRESSION-- > -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [drill] paul-rogers commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
paul-rogers commented on a change in pull request #1892: DRILL-7437: Storage
Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r390541287
##
File path:
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java
##
@@ -0,0 +1,135 @@
+/*
+ * 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.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.drill.shaded.guava.com.google.common.base.MoreObjects;
+
+import java.util.Arrays;
+import java.util.Map;
+import java.util.Objects;
+
+public class HttpAPIConfig {
+
+ private final String url;
+
+ private final String method;
+
+ private final Map headers;
+
+ private final String authType;
+
+ private final String userName;
+
+ private final String password;
+
+ private final String postBody;
+
+ public HttpAPIConfig(@JsonProperty("url") String url,
+ @JsonProperty("method") String method,
+ @JsonProperty("headers") Map headers,
+ @JsonProperty("authType") String authType,
+ @JsonProperty("userName") String userName,
+ @JsonProperty("password") String password,
+ @JsonProperty("postBody") String postBody) {
+
+// Get the request method. Only accept GET and POST requests. Anything
else will default to GET.
+if (method.toLowerCase().equals("get") ||
method.toLowerCase().equals("post")) {
+ this.method = method.toLowerCase();
+} else {
+ this.method = "get";
+}
+this.headers = headers;
+
+// Put a trailing slash on the URL if it is missing
+if (url.charAt(url.length() - 1) != '/') {
+ this.url = url + "/";
+} else {
+ this.url = url;
+}
Review comment:
Thinking how this works: The JSON comes from either 1) the user, or 2)
persistent storage. If from the user, we will doctor up the values in this
constructor, and the result will be converted back to JSON in the persistent
store. If from the persistent store (older version wrote the JSON), then the
form used in-memory won't match the persistent version until the next update.
I wonder, would it be better to do this fix-up when creating the reader
rather than in the config ctor?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
paul-rogers commented on a change in pull request #1892: DRILL-7437: Storage
Plugin for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407237465
##
File path:
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java
##
@@ -0,0 +1,148 @@
+/*
+ * 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.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.drill.common.PlanStringBuilder;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+import java.util.Objects;
+
+public class HttpAPIConfig {
+ private static final Logger logger =
LoggerFactory.getLogger(HttpAPIConfig.class);
+
+ private final String url;
+
+ private final String method;
+
+ private final Map headers;
+
+ private final String authType;
+
+ private final String userName;
+
+ private final String password;
+
+ private final String postBody;
+
+ public HttpAPIConfig(@JsonProperty("url") String url,
+ @JsonProperty("method") String method,
+ @JsonProperty("headers") Map headers,
+ @JsonProperty("authType") String authType,
+ @JsonProperty("userName") String userName,
+ @JsonProperty("password") String password,
+ @JsonProperty("postBody") String postBody) {
+
+this.headers = headers;
+this.method = Strings.isNullOrEmpty(method)
+? "GET" : method.trim().toUpperCase();
+
+// Get the request method. Only accept GET and POST requests. Anything
else will default to GET.
+switch (this.method) {
+ case "GET":
Review comment:
Since this is a plugin config, we want to keep types as a simple as
possible. Using a string for the method is fine.
An open question is whether validation should occur here or at the point
where the information is used. If here, then we'll reject a badly-formed method
when the user tries to edit it in the UI. However, we'll also prevent the
Drillbit from starting if that bad JSON makes it to persistent storage (or we
change the format later on.)
For now, doing validation here is fine until we come up with a uniform
solution for all configs.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin for Generic HTTP REST API
cgivre commented on a change in pull request #1892: DRILL-7437: Storage Plugin
for Generic HTTP REST API
URL: https://github.com/apache/drill/pull/1892#discussion_r407238496
##
File path:
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java
##
@@ -0,0 +1,148 @@
+/*
+ * 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.drill.exec.store.http;
+
+import com.fasterxml.jackson.annotation.JsonProperty;
+import org.apache.drill.common.PlanStringBuilder;
+import org.apache.drill.common.exceptions.UserException;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+import java.util.Objects;
+
+public class HttpAPIConfig {
+ private static final Logger logger =
LoggerFactory.getLogger(HttpAPIConfig.class);
+
+ private final String url;
+
+ private final String method;
+
+ private final Map headers;
+
+ private final String authType;
+
+ private final String userName;
+
+ private final String password;
+
+ private final String postBody;
+
+ public HttpAPIConfig(@JsonProperty("url") String url,
+ @JsonProperty("method") String method,
+ @JsonProperty("headers") Map headers,
+ @JsonProperty("authType") String authType,
+ @JsonProperty("userName") String userName,
+ @JsonProperty("password") String password,
+ @JsonProperty("postBody") String postBody) {
+
+this.headers = headers;
+this.method = Strings.isNullOrEmpty(method)
+? "GET" : method.trim().toUpperCase();
+
+// Get the request method. Only accept GET and POST requests. Anything
else will default to GET.
+switch (this.method) {
+ case "GET":
Review comment:
@paul-rogers
I do think there should be some means of verifying plugin config, ideally
with a helpful error message when the user attempts to submit an invalid plugin
config. Right now, unfortunately, all they get is an error message saying
somthing like `Invalid JSON` with no real way of determining the cause.
The syntax highlighter helps if it is an actual syntax error, but for
invalid configs, the user really has no way of knowing. This really isn't for
this PR, but something to think about.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[jira] [Created] (DRILL-7698) RDBMS Plugin Not Returning Results from Presto
Charles Givre created DRILL-7698:
Summary: RDBMS Plugin Not Returning Results from Presto
Key: DRILL-7698
URL: https://issues.apache.org/jira/browse/DRILL-7698
Project: Apache Drill
Issue Type: Bug
Components: Storage - JDBC
Affects Versions: 1.18.0
Reporter: Charles Givre
Attachments: Screen Shot 2020-04-12 at 2.43.33 PM.png, Screen Shot
2020-04-12 at 2.56.10 PM.png, Screen Shot 2020-04-12 at 3.00.00 PM.png, Screen
Shot 2020-04-12 at 3.01.37 PM.png
Using the RDBMS storage plugin, Drill is unable to connect to Presto. More
specifically, Drill seems to be connecting and sending queries to Presto, but
then nothing happens with the query results.
I verified the configuration using DBBeaver and was able to successfully query
Presto. See screenshot below for config.
!Screen Shot 2020-04-12 at 2.43.33 PM.png!
Presto ships with a few sample databases as shown below, and these should be
visible in Drill but are not.
!Screen Shot 2020-04-12 at 2.56.10 PM.png!
>From the logs below, Presto is clearly receiving the queries from Drill, and
>the queries are returning results, but Drill seems to be dropping the results.
> While this may seem like a silly exercise, querying Presto from Drill, the
>fact that it didn't work makes me think we may have a bug in the JDBC Storage
>Plugin.
!Screen Shot 2020-04-12 at 3.01.37 PM.png!
!Screen Shot 2020-04-12 at 3.00.00 PM.png!
h2. Steps to Reproduce
1. Download and start Docker container with Presto.
2. Download Presto JDBC driver (https://prestodb.io/download.html) and copy to
Drill classpath.
3. Create RDBMS storage plugin instance using default config below:
{code:java}
{
"type": "jdbc",
"driver": "io.prestosql.jdbc.PrestoDriver",
"url": "jdbc:presto://localhost:8080/tpch/sf1",
"username": "user",
"password": null,
"caseInsensitiveTableNames": true,
"sourceParameters": {},
"enabled": true
}
{code}
4. Execute a SHOW DATABASES query and you will see that no presto related
results are returned. Various queries to the INFORMATION SCHEMA reveal the
same thing.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Created] (DRILL-7699) javax.validation dependency is old
Rafael Jaimes created DRILL-7699: Summary: javax.validation dependency is old Key: DRILL-7699 URL: https://issues.apache.org/jira/browse/DRILL-7699 Project: Apache Drill Issue Type: Bug Components: Client - JDBC Affects Versions: 1.17.0 Reporter: Rafael Jaimes Fix For: Future The dependency for the validation-api being called in the pom of exec/jdbc is quite old and causes a failure in Presto when loading the JDBC driver. Other programs might have trouble loading the JDBC driver. Changing the version to 2.0.1-Final fixes the problem. It might also be feasible to just not specify the version at all. javax.validation validation-api 1.1.0.Final -- This message was sent by Atlassian Jira (v8.3.4#803005)
[GitHub] [drill] cgivre opened a new pull request #2053: DRILL-7699: Update javax.validation Dependency
cgivre opened a new pull request #2053: DRILL-7699: Update javax.validation Dependency URL: https://github.com/apache/drill/pull/2053 # [DRILL-7699](https://issues.apache.org/jira/browse/DRILL-7699): Update javax.validation Dependency ## Description This PR fixes a bug by updating the `javax.validation` dependency to the most current version. ## Documentation No user visible changes. ## Testing Ran existing unit tests for `jdbc` package. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2051: DRILL-7696: EVF v2 scan schema resolution
paul-rogers commented on a change in pull request #2051: DRILL-7696: EVF v2
scan schema resolution
URL: https://github.com/apache/drill/pull/2051#discussion_r407251443
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/TupleSchema.java
##
@@ -161,6 +161,17 @@ public boolean isEquivalent(TupleMetadata other) {
return true;
}
+ @Override
+ public boolean equals(Object o) {
Review comment:
See above.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #2051: DRILL-7696: EVF v2 scan schema resolution
paul-rogers commented on a change in pull request #2051: DRILL-7696: EVF v2
scan schema resolution
URL: https://github.com/apache/drill/pull/2051#discussion_r407251403
##
File path:
exec/vector/src/main/java/org/apache/drill/exec/record/metadata/DynamicColumn.java
##
@@ -0,0 +1,87 @@
+/*
+ * 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.drill.exec.record.metadata;
+
+import java.util.Objects;
+
+import org.apache.drill.common.types.TypeProtos.DataMode;
+import org.apache.drill.common.types.TypeProtos.MinorType;
+import org.apache.drill.exec.record.MaterializedField;
+
+/**
+ * A dynamic column has a name but not a type. The column may be
+ * a map, array or scalar: we don't yet know. A dynamic column is
+ * the equivalent of an item in a name-only project list. This type
+ * can also represent a wildcard. A dynamic column is not a concrete
+ * data description: it must be resolved to an actual type before
+ * it can be used to create vectors, readers, writers, etc. The
+ * dynamic column allows the tuple metadata to be used to represent
+ * all phases of a schema lifecycle, including Drill's "dynamic"
+ * schema before a reader resolves the column to some actual
+ * type.
+ */
+public class DynamicColumn extends AbstractColumnMetadata {
+
+ // Same as SchemaPath.DYNAMIC_STAR, but SchemaPath is not visible here.
+ public static final String WILDCARD = "**";
+ public static final DynamicColumn WILDCARD_COLUMN = new
DynamicColumn(WILDCARD);
+
+ public DynamicColumn(String name) {
+super(name, MinorType.LATE, DataMode.REQUIRED);
+ }
+
+ @Override
+ public StructureType structureType() { return StructureType.DYNAMIC; }
+
+ @Override
+ public boolean isDynamic() { return true; }
+
+ @Override
+ public MaterializedField schema() {
+return MaterializedField.create(name, majorType());
+ }
+
+ @Override
+ public MaterializedField emptySchema() {
+return schema();
+ }
+
+ @Override
+ public ColumnMetadata cloneEmpty() {
+return copy();
+ }
+
+ @Override
+ public ColumnMetadata copy() {
+return new DynamicColumn(name);
+ }
+
+ @Override
+ public boolean equals(Object o) {
Review comment:
AFAIK, we only need hash code if the objects will be keys, which these
should not be. (The name can be a key, but the whole object won't be.)
Your comment pointed out that we don't have an `equals()` methods in these
classes. The original code punted and used the code from `MaterializedField`.
(I probably wrote that code.) Fixed that.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers opened a new pull request #2054: DRILL-6168: Revise format plugin table functions
paul-rogers opened a new pull request #2054: DRILL-6168: Revise format plugin table functions URL: https://github.com/apache/drill/pull/2054 # [DRILL-6168](https://issues.apache.org/jira/browse/DRILL-6168): Revise format plugin table functions ## Description [DRILL-7612](https://issues.apache.org/jira/browse/DRILL-7612) notes a conflict with how we handle the properties of format plugins. This PR fixes that conflict. On the one hand, we want all plugins (storage and format) to be immutable as they are used as keys in internal maps. Those internal maps will become invalid if we change the values of any plugin property used to compute the hash value. On the other hand, format plugins participate in table functions. The table function implementation is based on Java introspection: it creates an empty instance, then sets public fields to the values requested in the table function. Clearly, these two use cases conflict: one requires immutability, the other mutability. The solution is to revise table functions to not require mutability. We observe the format plugins are JSON-serializable by definition. Jackson allows serializing an object to and from a generic JSON object. This becomes the foundation for the new implementation. A side benefit is that we can start with the existing plugin config values (DRILL-6168): * If an existing format plugin is available, convert it to a generic JSON object, else create an empty object. * Apply each table function parameter as a key/value pair to this object, overwriting properties obtained above. * Convert the resulting object into a new format plugin config object. With this change, we can now change every format plugin to make fields immutable. This requires adding a `@JsonCreator` constructor and sometimes "getter" functions. Also used this opportunity to clean up `equals()` and `hashCode()` functions, and add `toString()` where missing. Doing this work, discovered that the `bootstrap-storage-plugins.json` file used the wrong (old) field name for the text format `fieldDelimiter` field: `delimiter`. Updated the file with the modern name, and added an alias to ensure the existing plugins will continue to load. ## Documentation None for end users. For developers, we must revise descriptions which say that fields should be mutable and public. ## Testing Reran all unit tests to verify no regressions. 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: us...@infra.apache.org With regards, Apache Git Services
