Addressed Review Comments

common/pom.xml

@@ -37,14 +37,12 @@
       <artifactId>drill-protocol</artifactId>
       <version>${project.version}</version>
     </dependency>
-
     <dependency>
       <!-- add as provided scope so that we can compile TestTools.  Should only be ever used in a test scenario where someone else is bringing JUnit in. -->
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
       <version>${junit.version}</version>
     </dependency>
-
     <dependency>
       <groupId>com.typesafe</groupId>
       <artifactId>config</artifactId>

contrib/storage-http/README.md

@@ -33,8 +33,8 @@ The `connection` can accept the following options:
 .  The format is:
 ```json
 headers: {
-   "key1":, "Value1",
-   "key2", "Value2"
+   "key1": "Value1",
+   "key2": "Value2"
 }
 ```
 * `authType`: If your API requires authentication, specify the authentication type. At the time of implementation, the plugin only supports basic authentication, however, the
@@ -74,45 +74,44 @@ FROM <plugin>.<connection>.<arguments>
 The API sunrise-sunset.org returns data in the following format:
 
  ```json
-   {
-         "results":
-         {
-           "sunrise":"7:27:02 AM",
-           "sunset":"5:05:55 PM",
-           "solar_noon":"12:16:28 PM",
-           "day_length":"9:38:53",
-           "civil_twilight_begin":"6:58:14 AM",
-           "civil_twilight_end":"5:34:43 PM",
-           "nautical_twilight_begin":"6:25:47 AM",
-           "nautical_twilight_end":"6:07:10 PM",
-           "astronomical_twilight_begin":"5:54:14 AM",
-           "astronomical_twilight_end":"6:38:43 PM"
-         },
-          "status":"OK"
-       }
-   }
+ "results":
+ {
+   "sunrise":"7:27:02 AM",
+   "sunset":"5:05:55 PM",
+   "solar_noon":"12:16:28 PM",
+   "day_length":"9:38:53",
+   "civil_twilight_begin":"6:58:14 AM",
+   "civil_twilight_end":"5:34:43 PM",
+   "nautical_twilight_begin":"6:25:47 AM",
+   "nautical_twilight_end":"6:07:10 PM",
+   "astronomical_twilight_begin":"5:54:14 AM",
+   "astronomical_twilight_end":"6:38:43 PM"
+ },
+  "status":"OK"
+}
 ```
 To query this API, set the configuration as follows:
 
 ```json
-{
+
  {
    "type": "http",
    "cacheResults": false,
    "enabled": true,
-   "timeout" 5,
+   "timeout": 5,
    "connections": {
      "sunrise": {
        "url": "https://api.sunrise-sunset.org/",
-       "method": "get",
+       "method": "GET",
        "headers": null,
        "authType": "none",
        "userName": null,
        "password": null,
        "postBody": null
      }
-   },
+   }
 }
+
 ```
 Then, to execute a query:
 ```sql
@@ -138,11 +137,11 @@ To connect Drill to JIRA Cloud, use the following configuration:
 {
   "type": "http",
   "cacheResults": false,
-  "timeout" 5,
+  "timeout": 5,
   "connections": {
     "sunrise": {
       "url": "https://api.sunrise-sunset.org/",
-      "method": "get",
+      "method": "GET",
       "headers": null,
       "authType": "none",
       "userName": null,
@@ -151,7 +150,7 @@ To connect Drill to JIRA Cloud, use the following configuration:
     },
     "jira": {
       "url": "https://<project>.atlassian.net/rest/api/3/",
-      "method": "get",
+      "method": "GET",
       "headers": {
         "Accept": "application/json"
       },
@@ -212,7 +211,8 @@ ORDER BY issue_count DESC
  3. This plugin does not implement filter pushdowns. Filter pushdown has the potential to improve performance.
 
  4. This plugin only reads JSON responses. Future functionality may include the ability to parse XML, CSV or other common rest responses.
-
+
+ 5. At this time `POST` bodies can only be in the format of KV pairs. Some APIs accept JSON based `POST` bodies and this is not currently supported.
 
 
 

contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpAPIConfig.java

@@ -19,13 +19,17 @@
 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 org.apache.drill.common.PlanStringBuilder;
+import org.apache.parquet.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.util.Arrays;
 import java.util.Map;
 import java.util.Objects;
 
 public class HttpAPIConfig {
+  private static final Logger logger = LoggerFactory.getLogger(HttpAPIConfig.class);
 
   private final String url;
 
@@ -49,12 +53,24 @@ public HttpAPIConfig(@JsonProperty("url") String url,
                        @JsonProperty("password") String password,
                        @JsonProperty("postBody") String postBody) {
 
+    if (method == null) {
+      method = "GET";
+    }
+
     // 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";
+    switch (method) {
+      case "get":
+      case "GET":
+      case "post":
+      case "POST":
+        this.method = method.toUpperCase();
+        break;
+      default:
+        // Case for null or other HTTP Request types
+        logger.warn("HTTP/REST Plugin only supports GET and POST requests. Current config for {} uses method {}.", url, method);
+        this.method = "GET";
     }
+
     this.headers = headers;
 
     // Put a trailing slash on the URL if it is missing
@@ -66,13 +82,10 @@ public HttpAPIConfig(@JsonProperty("url") String url,
 
     // Get the authentication method. Future functionality will include OAUTH2 authentication but for now
     // Accept either basic or none.  The default is none.
-    this.authType = (authType == null || authType.isEmpty()) ? "none" : authType;
+    this.authType = Strings.isNullOrEmpty(authType) ? "none" : authType;
     this.userName = userName;
     this.password = password;
     this.postBody = postBody;
-
-    // Validate the authentication type
-
   }
 
   @JsonProperty("url")
@@ -104,14 +117,14 @@ public int hashCode() {
 
   @Override
   public String toString() {
-    return MoreObjects.toStringHelper(this)
-      .add("url", url)
-      .add("method", method)
-      .add("headers", headers)
-      .add("authType", authType)
-      .add("username", userName)
-      .add("password", password)
-      .add("postBody", postBody)
+    return new PlanStringBuilder(this)
+      .field("url", url)
+      .field("method", method)
+      .field("headers", headers)
+      .field("authType", authType)
+      .field("username", userName)
+      .field("password", password)
+      .field("postBody", postBody)
       .toString();
   }
 

contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpGroupScan.java

@@ -17,14 +17,14 @@
  */
 package org.apache.drill.exec.store.http;
 
-import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
 
 import com.fasterxml.jackson.annotation.JsonCreator;
 import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import com.fasterxml.jackson.annotation.JsonTypeName;
+import org.apache.drill.common.PlanStringBuilder;
 import org.apache.drill.common.expression.SchemaPath;
 
 import org.apache.drill.exec.physical.base.AbstractGroupScan;
@@ -33,17 +33,15 @@
 import org.apache.drill.exec.physical.base.ScanStats;
 import org.apache.drill.exec.physical.base.ScanStats.GroupScanProperty;
 import org.apache.drill.exec.physical.base.SubScan;
+import org.apache.drill.exec.planner.cost.DrillCostBase;
 import org.apache.drill.exec.proto.CoordinationProtos.DrillbitEndpoint;
-import org.apache.drill.shaded.guava.com.google.common.base.MoreObjects;
 import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
+
 
 @JsonTypeName("http-scan")
 public class HttpGroupScan extends AbstractGroupScan {
-  private static final Logger logger = LoggerFactory.getLogger(HttpGroupScan.class);
 
-  private List<SchemaPath> columns;
+  private final List<SchemaPath> columns;
   private final HttpScanSpec httpScanSpec;
   private final HttpStoragePluginConfig config;
 
@@ -65,6 +63,13 @@ public HttpGroupScan(HttpGroupScan that) {
     columns = that.getColumns();
   }
 
+  public HttpGroupScan(HttpGroupScan that, List<SchemaPath> columns) {
+    super("no-user");
+    this.columns = columns;
+    this.config = that.config;
+    this.httpScanSpec = that.httpScanSpec;
+  }
+
   @JsonCreator
   public HttpGroupScan(
     @JsonProperty("config") HttpStoragePluginConfig config,
@@ -88,7 +93,8 @@ public HttpGroupScan(
 
   @Override
   public void applyAssignments(List<DrillbitEndpoint> endpoints) {
-    logger.debug("HttpGroupScan applyAssignments");
+    // No filter pushdowns yet, so this method does nothing
+    return;
   }
 
   @Override
@@ -104,15 +110,12 @@ public boolean canPushdownProjects(List<SchemaPath> columns) {
 
   @Override
   public SubScan getSpecificScan(int minorFragmentId) {
-    logger.debug("HttpGroupScan getSpecificScan");
     return new HttpSubScan(config, httpScanSpec, columns);
   }
 
   @Override
   public GroupScan clone(List<SchemaPath> columns) {
-    logger.debug("HttpGroupScan clone {}", columns);
-    HttpGroupScan newScan = new HttpGroupScan(this);
-    newScan.columns = columns;
+    HttpGroupScan newScan = new HttpGroupScan(this, columns);
     return newScan;
   }
 
@@ -129,26 +132,28 @@ public PhysicalOperator getNewWithChildren(List<PhysicalOperator> children) {
 
   @Override
   public ScanStats getScanStats() {
-    int colCount = columns.size();
     int estRowCount = 1;
-    int estDataSize = estRowCount * 200 * colCount;
-    int estCpuCost = 1;
+
+    int rowWidth = 200;
+    if (columns.size() == 0) { rowWidth = 100; }
+
+    int estDataSize = estRowCount * 200 * rowWidth;
+    int estCpuCost = DrillCostBase.PROJECT_CPU_COST;
     return new ScanStats(GroupScanProperty.NO_EXACT_ROW_COUNT,estRowCount, estCpuCost, estDataSize);
   }
 
   @Override
   public String toString() {
-    return MoreObjects.toStringHelper(this)
-      .add("httpScanSpec", httpScanSpec)
-      .add("columns", columns)
-      .add("httpStoragePluginConfig", config)
+    return new PlanStringBuilder(this)
+      .field("httpScanSpec", httpScanSpec)
+      .field("columns", columns)
+      .field("httpStoragePluginConfig", config)
       .toString();
   }
 
   @Override
   public int hashCode() {
-    return Arrays.hashCode(
-      new Object[]{httpScanSpec, columns, config});
+    return Objects.hash(httpScanSpec, columns, config);
   }
 
   @Override

contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpRecordReader.java

@@ -18,6 +18,7 @@
 
 package org.apache.drill.exec.store.http;
 
+import org.apache.drill.common.exceptions.UserException;
 import org.apache.drill.common.expression.SchemaPath;
 import org.apache.drill.exec.ops.FragmentContext;
 import org.apache.drill.exec.store.easy.json.JSONRecordReader;
@@ -45,8 +46,9 @@ public HttpRecordReader(FragmentContext context, List<SchemaPath> projectedColum
   /**
    * Executes the HTTP request and returns an InputStream to the retrieved data
    * @return InputStream the InputStream containing the data
+   * @throws UserException Throws a UserException if unable to return an open InputStream.
    */
-  private InputStream getInputStream() {
+  private InputStream getInputStream() throws UserException {
     String url = subScan.getFullURL();
     return http.getInputStream(url);
   }

contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/HttpScanBatchCreator.java

@@ -17,6 +17,7 @@
  */
 package org.apache.drill.exec.store.http;
 
+import java.util.ArrayList;
 import java.util.List;
 
 import org.apache.drill.common.exceptions.ExecutionSetupException;
@@ -28,7 +29,6 @@
 import org.apache.drill.exec.record.RecordBatch;
 import org.apache.drill.exec.store.RecordReader;
 import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
-import org.apache.drill.shaded.guava.com.google.common.collect.Lists;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -41,12 +41,9 @@ public ScanBatch getBatch(ExecutorFragmentContext context, HttpSubScan subScan,
     Preconditions.checkArgument(children == null || children.isEmpty());
 
     HttpStoragePluginConfig config = subScan.config();
-    List<RecordReader> readers = Lists.newArrayList();
-    List<SchemaPath> columns;
+    List<RecordReader> readers = new ArrayList<>();
 
-    if ((columns = subScan.columns()) == null) {
-      columns = GroupScan.ALL_COLUMNS;
-    }
+    List<SchemaPath> columns = subScan.columns() == null ? GroupScan.ALL_COLUMNS : subScan.columns();
     readers.add(new HttpRecordReader(context, columns, config, subScan));
 
     return new ScanBatch(subScan, context, readers);