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 2020/02/07 07:28:59 UTC
[GitHub] [drill] paul-rogers opened a new pull request #1971: DRILL-7572:
JSON structure parser
paul-rogers opened a new pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971
Provides a structure-driven parser for JSON. Provides an intermediate
level beteen the Jackson JSON parser and the "shims" that write to
Drill value vectors via the EVF.
## Description
DRILL-6953, PR #1913 provides an EVF-version of the JSON reader. The original plan was to commit a first draft, with the feature disabled, then evolve from there. Review comments suggested we ensure that the first PR pass all unit tests with the feature enabled. Another comment suggested the JSON parser portion be reusable for other format or storage plugins. All good ideas.
The amount of change in the original PR was becoming too large. So, we'll go ahead and split the work into smaller portions.
This ticket is for the "JSON structure parser." This parser accepts the token stream from the Jackson parser and provides a framework to create a tree structure that describes the data. To keep the code simple, to simplify tests, and to make review easier, the structure parser does not directly integrate with the rest of Drill. Instead, it provides a number of listener interfaces which the next level implements. (The next level will be in another PR.)
Note that, in this PR, the structure parser is an "orpjan": no other code uses it yet.
## Documentation
This is not a user-visible feature. When integrated into the JSON parser, the code remove a few existing restrictions when reading JSON. Additional documentation will be provided in that future PR.
## Testing
Detailed unit tests include. Reran all existing and new unit tests.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378190159
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/RootParser.java
##########
@@ -0,0 +1,121 @@
+/*
+ * 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.easy.json.parser;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.fasterxml.jackson.core.JsonToken;
+
+/**
+ * The root parsers are special: they must detect EOF. Drill supports
+ * top-level objects either enclosed in an array (which forms legal
+ * JSON), or as a series JSON objects (which is a common, if not
+ * entirely legal, form of JSON.)
+ */
+public abstract class RootParser implements ElementParser {
+ protected static final Logger logger = LoggerFactory.getLogger(RootParser.class);
+
+ private final JsonStructureParser structParser;
+ protected final ObjectParser rootObject;
+
+ public RootParser(JsonStructureParser structParser) {
+ this.structParser = structParser;
+ this.rootObject = new ObjectParser(this, structParser.options().rootListener);
+ }
+
+ public abstract boolean parseRoot(TokenIterator tokenizer);
+
+ @Override
+ public void parse(TokenIterator tokenizer) {
+ throw new UnsupportedOperationException();
+ }
+
+ protected boolean parseRootObject(JsonToken token, TokenIterator tokenizer) {
+ // Position: ^ ?
+ switch (token) {
+ case NOT_AVAILABLE:
+ return false; // Should never occur
+
+ case START_OBJECT:
+ // Position: { ^
+ rootObject.parse(tokenizer);
+ break;
+
+ default:
+ // Position ~{ ^
+ // Not a valid object.
+ // Won't actually get here: the Jackson parser prevents it.
+ throw errorFactory().syntaxError(token); // Nothing else is valid
+ }
+ return true;
+ }
+
+ protected ErrorFactory errorFactory() {
+ return structParser.errorFactory();
+ }
+
+ @Override
+ public ElementParser parent() { return null; }
+
+ @Override
+ public JsonStructureParser structParser() { return structParser; }
+
+ public static class RootObjectParser extends RootParser {
+
+ public RootObjectParser(JsonStructureParser structParser) {
+ super(structParser);
+ }
+
+ @Override
+ public boolean parseRoot(TokenIterator tokenizer) {
+ JsonToken token = tokenizer.next();
+ if (token == null) {
+ // Position: EOF ^
+ return false;
+ } else {
+ return parseRootObject(token, tokenizer);
+ }
+ }
+ }
+
+ public static class RootArrayParser extends RootParser {
+
+ public RootArrayParser(JsonStructureParser structParser) {
+ super(structParser);
+ }
+
+ @Override
+ public boolean parseRoot(TokenIterator tokenizer) {
+ JsonToken token = tokenizer.next();
+ if (token == null) {
+ // Position: { ... EOF ^
+ // Saw EOF, but no closing ]. Warn and ignore.
+ // Note that the Jackson parser won't let us get here;
+ // it will have already thrown a syntax error.
+ logger.warn("Failed to close outer array. {}",
+ tokenizer.context());
+ return false;
+ } else if (token == JsonToken.END_ARRAY) {
+ return false;
+ } else {
+ return parseRootObject(token, tokenizer);
+ }
+ }
+ }
+}
Review comment:
Please add empty line.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378173853
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureParser.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.easy.json.parser;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootArrayParser;
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootObjectParser;
+import org.apache.drill.exec.store.easy.json.parser.TokenIterator.RecoverableJsonException;
+import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class JsonStructureParser {
+ protected static final Logger logger = LoggerFactory.getLogger(JsonStructureParser.class);
+
+ final JsonParser parser;
+ private final JsonStructureOptions options;
+ private final TokenIterator tokenizer;
+ private final RootParser rootState;
+ private int errorRecoveryCount;
+
+ public JsonStructureParser(InputStream stream, JsonStructureOptions options) {
+ Preconditions.checkNotNull(options);
+ Preconditions.checkNotNull(options.rootListener);
+ Preconditions.checkNotNull(options.errorFactory);
+ this.options = options;
+ try {
+ ObjectMapper mapper = new ObjectMapper()
+ .configure(JsonParser.Feature.ALLOW_COMMENTS, true)
+ .configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
+ if (options.allowNanInf) {
+ mapper.configure(JsonParser.Feature.ALLOW_NON_NUMERIC_NUMBERS, true);
+ }
Review comment:
It may be replaced with
```
mapper.configure(JsonParser.Feature.ALLOW_NON_NUMERIC_NUMBERS, options.allowNanInf);
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378172073
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
+
+ /**
+ * Allow Infinity and NaN for float values.
+ */
+
+ public boolean allowNanInf;
+
+ /**
+ * Describes whether or not this reader can unwrap a single root array record
+ * and treat it like a set of distinct records.
+ */
+ public boolean skipOuterList = true;
+ public boolean skipMalformedRecords;
+
+ /**
+ * Top-level event listeners.
+ */
+ public ObjectListener rootListener;
Review comment:
I don't think that we should store `ObjectListener` and `ErrorFactory` in the holder of the options.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r379386297
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
Review comment:
Thanks for pointing to the existing `JsonOptions` class. Making this object immutable would help to avoid occasional changing of the option values.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378182270
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureParser.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.easy.json.parser;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootArrayParser;
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootObjectParser;
+import org.apache.drill.exec.store.easy.json.parser.TokenIterator.RecoverableJsonException;
+import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class JsonStructureParser {
+ protected static final Logger logger = LoggerFactory.getLogger(JsonStructureParser.class);
+
+ final JsonParser parser;
+ private final JsonStructureOptions options;
+ private final TokenIterator tokenizer;
+ private final RootParser rootState;
+ private int errorRecoveryCount;
+
+ public JsonStructureParser(InputStream stream, JsonStructureOptions options) {
+ Preconditions.checkNotNull(options);
+ Preconditions.checkNotNull(options.rootListener);
+ Preconditions.checkNotNull(options.errorFactory);
+ this.options = options;
+ try {
+ ObjectMapper mapper = new ObjectMapper()
+ .configure(JsonParser.Feature.ALLOW_COMMENTS, true)
+ .configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
+ if (options.allowNanInf) {
+ mapper.configure(JsonParser.Feature.ALLOW_NON_NUMERIC_NUMBERS, true);
+ }
+
+ parser = mapper.getFactory().createParser(stream);
+ } catch (JsonParseException e) {
+ throw errorFactory().parseError("Failed to create the JSON parser", e);
+ } catch (IOException e) {
+ throw errorFactory().ioException(e);
+ }
+ tokenizer = new TokenIterator(parser, options, errorFactory());
+ rootState = makeRootState();
+ }
+
+ public ErrorFactory errorFactory() { return options.errorFactory; }
+
+ public JsonStructureOptions options() { return options; }
+
+ private RootParser makeRootState() {
+ JsonToken token = tokenizer.next();
+ if (token == null) {
+ return null;
+ }
+ switch (token) {
+
+ // File contains an array of records.
+ case START_ARRAY:
+ if (options.skipOuterList) {
+ return new RootArrayParser(this);
+ } else {
+ throw errorFactory().structureError(
+ "JSON includes an outer array, but outer array support is not enabled");
+ }
+
+ // File contains a sequence of one or more records,
+ // presumably sequentially.
+ case START_OBJECT:
+ tokenizer.unget(token);
+ return new RootObjectParser(this);
+
+ // Not a valid JSON file for Drill.
+ // Won't get here because the Jackson parser catches errors.
+ default:
+ throw errorFactory().syntaxError(token);
+ }
+ }
+
+ public boolean next() {
+ if (rootState == null) {
+ // Only occurs for an empty document
+ return false;
+ }
+ for (;;) {
+ try {
+ return rootState.parseRoot(tokenizer);
+ } catch (RecoverableJsonException e) {
+ if (! recover()) {
+ return false;
+ }
+ }
+ }
+ }
+
+ /**
+ * Attempt recovery from a JSON syntax error by skipping to the next
+ * record. The Jackson parser is quite limited in its recovery abilities.
+ *
+ * @return {@code true{@code if another record can be read, {@code false}
Review comment:
Please fix the tags.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #1971: DRILL-7572: JSON
structure parser
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #1971: DRILL-7572: JSON
structure parser
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#issuecomment-587300916
@vvysotskyi, thanks for pointing out the question; I missed it when reading the code comments.
Looked at `CountingJsonReader`. Looks like creates a series or rows, one per input row, with just a bit field set to 1. This reader could do exactly the same by projecting none of the columns and instead writing that bit = 1 value for the start of each top-level object. The non-projected columns will "free-wheel" over the incoming JSON.
A better solution is to actually return the count. Maybe we need another option on the format plugin, `supportsCountPushDown()` so that we return the per-file row count rather grind through the effort of making trivial rows.
EVF has support for this idea with its notion of "project none" which occurs when the scan asks for now rows as in a `COUNT(*)`.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r379387581
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
+
+ /**
+ * Allow Infinity and NaN for float values.
+ */
+
+ public boolean allowNanInf;
+
+ /**
+ * Describes whether or not this reader can unwrap a single root array record
+ * and treat it like a set of distinct records.
+ */
+ public boolean skipOuterList = true;
+ public boolean skipMalformedRecords;
+
+ /**
+ * Top-level event listeners.
+ */
+ public ObjectListener rootListener;
Review comment:
If this class is designed as an argument holder for JSON readers, let's rename it to something like `JsonContext`, so there wouldn't be an assumption that it should store only options for JSON.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378171613
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
Review comment:
Could you please make fields private final and add getters for them.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on issue #1971: DRILL-7572: JSON
structure parser
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on issue #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#issuecomment-587592016
@vvysotskyi, thanks for the explanation on the Calcite rule. I'll take a look at that as part of the new JSON reader work.
Thanks also for pointing out that I hadn't pushed the changes. I've now done so.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r378173230
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureParser.java
##########
@@ -0,0 +1,183 @@
+/*
+ * 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.easy.json.parser;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootArrayParser;
+import org.apache.drill.exec.store.easy.json.parser.RootParser.RootObjectParser;
+import org.apache.drill.exec.store.easy.json.parser.TokenIterator.RecoverableJsonException;
+import org.apache.drill.shaded.guava.com.google.common.base.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.fasterxml.jackson.core.JsonParseException;
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.core.JsonToken;
+import com.fasterxml.jackson.databind.ObjectMapper;
+
+public class JsonStructureParser {
+ protected static final Logger logger = LoggerFactory.getLogger(JsonStructureParser.class);
+
+ final JsonParser parser;
+ private final JsonStructureOptions options;
+ private final TokenIterator tokenizer;
+ private final RootParser rootState;
+ private int errorRecoveryCount;
+
+ public JsonStructureParser(InputStream stream, JsonStructureOptions options) {
+ Preconditions.checkNotNull(options);
Review comment:
It may be combined with assigning the value:
```suggestion
this.options = Preconditions.checkNotNull(options);
```
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r379093579
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
Review comment:
This is an analogy of the existing `JsonOptions` class which seems to be designed for this pattern:
```
options.foo = optionManager.getSomething(SOME_CONSTANT);
```
We can certainly turn this into an immutable object/builder pair. Is it worth doing for the very limited use cases we have?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on issue #1971: DRILL-7572: JSON
structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on issue #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#issuecomment-587353797
@paul-rogers, thanks for the explanation, it is great that EVF will handle such case without additional changes.
Regarding `supportsCountPushDown()`, for Parquet we already have a rule which replaces scan with a count by the direct scan with the value from the metadata. The complexity appears when we have count by column. In this case, only statistics may help. The rule mentioned above is `ConvertCountToDirectScanPrule`.
The good news is that when Metastore will be supported for other non-parquet format plugins, it became possible to use its metadata for such optimizations.
By the way, looks like changes for the first CR round weren't pushed.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r376367486
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/ArrayParser.java
##########
@@ -0,0 +1,57 @@
+/*
+ * 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.easy.json.parser;
+
+import com.fasterxml.jackson.core.JsonToken;
+
+public class ArrayParser extends AbstractElementParser {
+
+ private final ArrayListener arrayListener;
+ private final ValueParser elementParser;
+
+ public ArrayParser(ValueParser parent, ArrayListener arrayListener, ValueListener elementListener) {
+ super(parent);
+ this.arrayListener = arrayListener;
+ this.elementParser = new ValueParser(this, "[]", elementListener);
+ }
+
+ public ValueParser elementParser() { return elementParser; }
+
+ /**
+ * Parses <code>[ ^ ((value)(, (value)* )? ]</code>
+ */
+ @Override
+ public void parse(TokenIterator tokenizer) {
+ arrayListener.onStart();
+ top: for (;;) {
+ // Position: [ (value, )* ^ ?
+ JsonToken token = tokenizer.requireNext();
+ switch (token) {
Review comment:
Please fix indentation
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r379097191
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/JsonStructureOptions.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.easy.json.parser;
+
+public class JsonStructureOptions {
+ public boolean allTextMode;
+
+ /**
+ * Allow Infinity and NaN for float values.
+ */
+
+ public boolean allowNanInf;
+
+ /**
+ * Describes whether or not this reader can unwrap a single root array record
+ * and treat it like a set of distinct records.
+ */
+ public boolean skipOuterList = true;
+ public boolean skipMalformedRecords;
+
+ /**
+ * Top-level event listeners.
+ */
+ public ObjectListener rootListener;
Review comment:
I dislike long constructor argument lists. But, I dislike debating code style even more, so changed.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on issue #1971: DRILL-7572: JSON
structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on issue #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#issuecomment-586243438
@paul-rogers, what about a question regarding `CountingJsonReader`?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [drill] vvysotskyi commented on a change in pull request #1971:
DRILL-7572: JSON structure parser
Posted by GitBox <gi...@apache.org>.
vvysotskyi commented on a change in pull request #1971: DRILL-7572: JSON structure parser
URL: https://github.com/apache/drill/pull/1971#discussion_r376361296
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/store/easy/json/parser/ArrayListener.java
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.easy.json.parser;
+
+/**
+ * Represents one level within an array. The first time the parser sees
+ * the array element, it will call one of the "Element" methods with the
+ * look-ahead values visible to the parser. Since JSON is flexible, later
+ * data shapes may not necessarily follow the first shape. The implementation
+ * must handle this or throw an error if not supported.
+ * <p>
+ * When creating a multi-dimensional array, each array level is built one
+ * by one. each will receive the same type information (decreased by one
+ * array level.)
+ * <p>
+ * Then, while parsing, the parser calls events on the start and end of the
+ * array, as well as on each element.
+ * <p>
+ * The array listener is an attribute of a value listener, represent the
+ * "arrayness" of that value, if the value allows an array.
+ */
+public interface ArrayListener {
+
+ void onStart();
Review comment:
Could you please add javadocs for these methods?
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services