You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/05/27 17:57:53 UTC
[GitHub] [drill] vdiravka opened a new pull request, #2566: DRILL-8236: Move HttpHelperFunctions to use JSON2 reader
vdiravka opened a new pull request, #2566:
URL: https://github.com/apache/drill/pull/2566
# [DRILL-8236](https://issues.apache.org/jira/browse/DRILL-8236): Move HttpHelperFunctions to use JSON2 reader
## Description
It updates HttpHelperFunctions to leverage JSON v2
## Documentation
NA
## Testing
TestHttpUDFFunctions still pass
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2566: DRILL-8236: Move HttpHelperFunctions to use JSON2 reader
Posted by GitBox <gi...@apache.org>.
cgivre merged PR #2566:
URL: https://github.com/apache/drill/pull/2566
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] vdiravka commented on a diff in pull request #2566: DRILL-8236: Move HttpHelperFunctions to use JSON2 reader
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a diff in pull request #2566: DRILL-8236: Move HttpHelperFunctions to use JSON2 reader
Posted by GitBox <gi...@apache.org>.
cgivre commented on code in PR #2566:
URL: https://github.com/apache/drill/pull/2566#discussion_r883898280
##########
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:
@vdiravka Thanks for this PR. Will the options here be the global JSON options defined in the Drill config? IE: If `allTextMode` is set globally, will that be passed down to the UDF?
##########
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:
Here I think is a little different. The endpoint may define `jsonOptions` that might differ from the global options. Can we pass that as well? I think there's a method actually in the http config to actually get that object.
--
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-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org