[GitHub] [drill] vdiravka commented on a diff in pull request #2566: DRILL-8236: Move HttpHelperFunctions to use JSON2 reader

[GitBox]

vdiravka commented on code in PR #2566:
URL: https://github.com/apache/drill/pull/2566#discussion_r883941032


##########
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/udfs/HttpHelperFunctions.java:
##########
@@ -52,22 +52,16 @@ public static class HttpGetFunction implements 
DrillSimpleFunc {
     OptionManager options;
 
     @Inject
-    DrillBuf buffer;
+    ResultSetLoader loader;
 
     @Workspace
-    org.apache.drill.exec.vector.complex.fn.JsonReader jsonReader;
+    
org.apache.drill.exec.store.easy.json.loader.JsonLoaderImpl.JsonLoaderBuilder 
jsonLoaderBuilder;
 
     @Override
     public void setup() {
-      jsonReader = new 
org.apache.drill.exec.vector.complex.fn.JsonReader.Builder(buffer)
-        .defaultSchemaPathColumns()
-        
.readNumbersAsDouble(options.getOption(org.apache.drill.exec.ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE).bool_val)
-        
.allTextMode(options.getOption(org.apache.drill.exec.ExecConstants.JSON_ALL_TEXT_MODE).bool_val)
-        
.enableNanInf(options.getOption(org.apache.drill.exec.ExecConstants.JSON_READER_NAN_INF_NUMBERS).bool_val)
-        .build();
-
-      jsonReader.setIgnoreJSONParseErrors(
-        
options.getBoolean(org.apache.drill.exec.ExecConstants.JSON_READER_SKIP_INVALID_RECORDS_FLAG));
+      jsonLoaderBuilder = new 
org.apache.drill.exec.store.easy.json.loader.JsonLoaderImpl.JsonLoaderBuilder()
+        .resultSetLoader(loader)
+        .standardOptions(options);

Review Comment:
   Yes. You can check, `JsonLoaderOptions` initialized from `OptionSet`:
   ```
       public JsonLoaderBuilder standardOptions(OptionSet optionSet) {
         this.options = new JsonLoaderOptions(optionSet);
         return this;
       }
   ```
   And I have checked and can confirm - `ExecConstants.JSON_ALL_TEXT_MODE` sets 
[JsonLoaderOptions.allTextMode](https://github.com/apache/drill/blob/master/exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/loader/JsonLoaderOptions.java#L87)



##########
contrib/storage-http/src/main/java/org/apache/drill/exec/store/http/udfs/HttpHelperFunctions.java:
##########
@@ -147,15 +142,9 @@ public static class HttpGetFromStoragePluginFunction 
implements DrillSimpleFunc
 
     @Override
     public void setup() {
-      jsonReader = new 
org.apache.drill.exec.vector.complex.fn.JsonReader.Builder(buffer)
-        .defaultSchemaPathColumns()
-        
.readNumbersAsDouble(options.getOption(org.apache.drill.exec.ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE).bool_val)
-        
.allTextMode(options.getOption(org.apache.drill.exec.ExecConstants.JSON_ALL_TEXT_MODE).bool_val)
-        
.enableNanInf(options.getOption(org.apache.drill.exec.ExecConstants.JSON_READER_NAN_INF_NUMBERS).bool_val)
-        .build();
-
-      jsonReader.setIgnoreJSONParseErrors(
-        
options.getBoolean(org.apache.drill.exec.ExecConstants.JSON_READER_SKIP_INVALID_RECORDS_FLAG));
+      jsonLoaderBuilder = new 
org.apache.drill.exec.store.easy.json.loader.JsonLoaderImpl.JsonLoaderBuilder()
+        .resultSetLoader(loader)
+        .standardOptions(options);

Review Comment:
   Answered in above comment.
   
   > You can check, JsonLoaderOptions initialized from OptionSet:
   ```
       public JsonLoaderBuilder standardOptions(OptionSet optionSet) {
         this.options = new JsonLoaderOptions(optionSet);
         return this;
       }
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscr...@drill.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org