[GitHub] [drill] cgivre commented on a change in pull request #2494: DRILL-8167: Add JSON Config Options to Format Config

[GitBox <gi...@apache.org>]

cgivre commented on a change in pull request #2494:
URL: https://github.com/apache/drill/pull/2494#discussion_r826328608



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/JSONRecordReader.java
##########
@@ -124,19 +139,76 @@ private JSONRecordReader(FragmentContext fragmentContext, Path inputPath, JsonNo
       this.embeddedContent = embeddedContent;
     }
 
+    // If the config is null, create a temporary one with the global options.
+    if (config == null) {
+      this.config = new JSONFormatConfig(null,
+        embeddedContent == null && fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_ALL_TEXT_MODE_VALIDATOR),
+        embeddedContent == null && fragmentContext.getOptions().getOption(ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE_VALIDATOR),
+        fragmentContext.getOptions().getOption(ExecConstants.JSON_SKIP_MALFORMED_RECORDS_VALIDATOR),
+        fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_ESCAPE_ANY_CHAR_VALIDATOR),
+        fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_NAN_INF_NUMBERS_VALIDATOR));
+    } else {
+      this.config = config;
+    }
+
     this.fileSystem = fileSystem;
     this.fragmentContext = fragmentContext;
-    // only enable all text mode if we aren't using embedded content mode.
-    this.enableAllTextMode = embeddedContent == null && fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_ALL_TEXT_MODE_VALIDATOR);
-    this.enableNanInf = fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_NAN_INF_NUMBERS_VALIDATOR);
-    this.enableEscapeAnyChar = fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_ESCAPE_ANY_CHAR_VALIDATOR);
-    this.readNumbersAsDouble = embeddedContent == null && fragmentContext.getOptions().getOption(ExecConstants.JSON_READ_NUMBERS_AS_DOUBLE_VALIDATOR);
+
+    this.enableAllTextMode = allTextMode();
+    this.enableNanInf = nanInf();
+    this.enableEscapeAnyChar = escapeAnyChar();
+    this.readNumbersAsDouble = readNumbersAsDouble();
     this.unionEnabled = embeddedContent == null && fragmentContext.getOptions().getBoolean(ExecConstants.ENABLE_UNION_TYPE_KEY);
-    this.skipMalformedJSONRecords = fragmentContext.getOptions().getOption(ExecConstants.JSON_SKIP_MALFORMED_RECORDS_VALIDATOR);
+    this.skipMalformedJSONRecords = skipMalformedJSONRecords();
     this.printSkippedMalformedJSONRecordLineNumber = fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_PRINT_INVALID_RECORDS_LINE_NOS_FLAG_VALIDATOR);
     setColumns(columns);
   }
 
+  /**
+   * Returns the value of the all text mode.  Values set in the format config will override global values.
+   * @return The value of allTextMode
+   */
+  private boolean allTextMode() {
+    // only enable all text mode if we aren't using embedded content mode.
+    if (config.getAllTextMode() == null) {
+      return embeddedContent == null && fragmentContext.getOptions().getOption(ExecConstants.JSON_READER_ALL_TEXT_MODE_VALIDATOR);

Review comment:
       @jnturton 
   Thanks for the comments.  I followed the pattern you suggested and now the precedence is SYSTEM < FORMAT < SESSION.  One thing to note is that I don't think you can override the session scope.  Drill doesn't seem to have the distinction between format and query.  But... this follows the pattern from parquet so at least it is consistent.




-- 
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