[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r103547010
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -170,6 +184,21 @@ public void ensureAtLeastOneField(ComplexWriter
writer) {
}
}
+ private void initializeFieldWriter(MapWriter fieldWriter, String path) {
+if (allTextMode) {
+ fieldWriter.varChar(path);
+} else {
+ fieldWriter.integer(path);
--- End diff --
At the Drill Hangout it was mentioned that this fix handles nulls for
non-Varchar fields. Note that it does so using the same mechanism that Drill
uses elsewhere: assume the field is of type int. However, we have many, many
bugs that result from that assumption. There is simply no guarantee that, in a
later batch, when we see the field, that it will, in fact, be an int.
I'm not sure whether it is OK to simply continue to propagate that
well-known error here (as we have done) or to take another path that avoids the
error. (Since doing so requires a design change that has, so far, always been
beyond our ability to accomplish.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user paul-rogers commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r101880961
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -63,6 +67,12 @@
private final List emptyArrayWriters = Lists.newArrayList();
/**
+ * Collection for tracking nullable fields during reading
+ * and storing them for creating default typed vectors
+ */
+ private Map, MapWriter> fieldPathWriter = Maps.newHashMap();
--- End diff --
Actually, in JSON, all scalar types can be null. Because JSON has no
schema, any field can have any type. Drill assumes that a given field has a
single type. But, in JSON semantics, that single type can be null. That is, the
following is always valid:
{code}
{ a: 10, b: null }
{ a: null, b: "foo" }
{ a: 20 }
{code}
JSON, but not Drill, differentiates between "not present" and null.
Given this, we don't need a special map for nullable fields: all JSON
fields are nullable.
Of course, JSON allows a null map, which Drill does not handle, but let's
ignore that here...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r101759433
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -492,7 +538,19 @@ private void writeDataAllText(MapWriter map,
FieldSelection selection,
}
}
map.end();
+ }
+ /**
+ * Puts copy of field path list to fieldPathWriter map.
+ * @param fieldName
+ */
+ private void putFieldPath(String fieldName, MapWriter map) {
--- End diff --
I attached results of testing on the last fix to the Jira.
https://drive.google.com/open?id=1l3Dg0DCV3p-OhwA0v6qdl2wGcezXDl5qaUqv56EEBn8
It should be mentioned that a lot of time goes to the creating fields and
them writers. So times of execution also compared with time of querying files,
in which nulls replaced by varchar.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r101624385
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -492,7 +538,19 @@ private void writeDataAllText(MapWriter map,
FieldSelection selection,
}
}
map.end();
+ }
+ /**
+ * Puts copy of field path list to fieldPathWriter map.
+ * @param fieldName
+ */
+ private void putFieldPath(String fieldName, MapWriter map) {
--- End diff --
Do you have the performance data that you had collected earlier with
different percentage of NULLs, after this change ? I still feel there could be
some further optimization needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r101024133
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -492,10 +537,18 @@ private void writeDataAllText(MapWriter map,
FieldSelection selection,
}
}
map.end();
-
}
/**
+ * Puts copy of field path list to fieldPathWriter map.
+ * @param fieldName
+ */
+ private void putFieldPath(String fieldName, MapWriter map) {
+List fieldPath = Lists.newArrayList(path);
--- End diff --
Thanks, fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user amansinha100 commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r100653436
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -492,10 +537,18 @@ private void writeDataAllText(MapWriter map,
FieldSelection selection,
}
}
map.end();
-
}
/**
+ * Puts copy of field path list to fieldPathWriter map.
+ * @param fieldName
+ */
+ private void putFieldPath(String fieldName, MapWriter map) {
+List fieldPath = Lists.newArrayList(path);
--- End diff --
Before allocating the fieldPath list, should we first check if it is
already present in the fieldPathWriter ? This function is called for every null
value which makes it quite expensive (as shown by your performance
experiments). If there are 1000 records of type {'x': null}, ideally we want
to call this method only for field 'x' once.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r92478836 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -510,10 +517,13 @@ private void writeDataAllText(MapWriter map, FieldSelection selection, case VALUE_NUMBER_FLOAT: case VALUE_NUMBER_INT: case VALUE_STRING: +removeNotNullColumn(fieldName); --- End diff -- To clean 'path' if it knows it is recovering from some errors, was added this line: https://github.com/Serhii-Harnyk/drill/blob/9ce6d56b46bcd540697c85cd2f280831dd50b277/exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java#L243. Call of the method removeNotNullColumn() is a try to optimize the final size of the map fieldPathWriter, by removing fields, which were added to the map and initializing at current iteration. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r92452185 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -510,10 +517,13 @@ private void writeDataAllText(MapWriter map, FieldSelection selection, case VALUE_NUMBER_FLOAT: case VALUE_NUMBER_INT: case VALUE_STRING: +removeNotNullColumn(fieldName); --- End diff -- I feel uncomfortable since the fix added 'removeNotNullColumn(fieldName)' on the code path that is supposed to be hotspot. Could the reader just clean 'path' if it knows it is recovering from some errors? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r90494185 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -59,12 +59,13 @@ private final ListVectorOutput listOutput; private final boolean extended = true; private final boolean readNumbersAsDouble; + private List path = Lists.newArrayList(); /** * Collection for tracking nullable fields during reading * and storing them for creating default typed vectors */ - private final Set nullableFields = Sets.newLinkedHashSet(); + private final Set> nullableFields = Sets.newLinkedHashSet(); --- End diff -- I did not see update on this. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r85984205 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -59,12 +59,13 @@ private final ListVectorOutput listOutput; private final boolean extended = true; private final boolean readNumbersAsDouble; + private List path = Lists.newArrayList(); /** * Collection for tracking nullable fields during reading * and storing them for creating default typed vectors */ - private final Set nullableFields = Sets.newLinkedHashSet(); + private final Set> nullableFields = Sets.newLinkedHashSet(); --- End diff -- Do you mean the iterating on line 163 needs the order to be preserved? I doubt it. Could you please double check? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r85975481
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -59,12 +59,13 @@
private final ListVectorOutput listOutput;
private final boolean extended = true;
private final boolean readNumbersAsDouble;
+ private List path = Lists.newArrayList();
--- End diff --
Since you are adding a stateful variable 'path', could you add a test with
some error json items injected in the middle to make sure it can still recover
and have good status when the option introduced in DRILL-4653 is enabled
('store.json.reader.skip_invalid_records') which will skip invalid tokens.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r84509937 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -337,6 +364,7 @@ private void writeData(MapWriter map, FieldSelection selection, boolean moveForw continue outside; } +nullableFields.remove(fieldName); --- End diff -- Fixed --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r84509944 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -418,6 +447,7 @@ private void writeDataAllText(MapWriter map, FieldSelection selection, boolean m continue outside; } + nullableFields.remove(fieldName); --- End diff -- Fixed --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r84507055 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -56,6 +58,12 @@ private final boolean readNumbersAsDouble; /** + * Collection for tracking nullable fields during reading + * and storing them for creating default typed vectors + */ + private final Set nullableFields = Sets.newLinkedHashSet(); --- End diff -- We need to store an order of nullable fields. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r84507516
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -131,14 +139,33 @@ public void ensureAtLeastOneField(ComplexWriter
writer) {
PathSegment fieldPath = fieldPathList.get(j);
if (emptyStatus.get(j)) {
if (allTextMode) {
- fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
--- End diff --
Added fixes for nested fields with handling "path" of the field.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user Serhii-Harnyk commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r84508112
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -131,14 +139,33 @@ public void ensureAtLeastOneField(ComplexWriter
writer) {
PathSegment fieldPath = fieldPathList.get(j);
if (emptyStatus.get(j)) {
if (allTextMode) {
- fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
+for (String fieldName : nullableFields) {
+ fieldWriter.varChar(fieldName);
+}
+ } else {
+fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ }
} else {
- fieldWriter.integer(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
--- End diff --
Test added
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r81627752 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -56,6 +58,12 @@ private final boolean readNumbersAsDouble; /** + * Collection for tracking nullable fields during reading + * and storing them for creating default typed vectors + */ + private final Set nullableFields = Sets.newLinkedHashSet(); --- End diff -- I don't see the reason you need it to be a LinkedHashSet. And if there is no particular reason for it, making it LinkedHashSet but not HashSet adds extra cost. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r81622069
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -131,14 +139,33 @@ public void ensureAtLeastOneField(ComplexWriter
writer) {
PathSegment fieldPath = fieldPathList.get(j);
if (emptyStatus.get(j)) {
if (allTextMode) {
- fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
--- End diff --
This is in the loop of going through fieldPathList, if there are two fields
in emptyStatus, will it result in calling checkNullFields twice and then
fieldWriter.varChar(fieldName) twice for the same field?
To make sure here we are doing the right thing, I think testing more on
hierarchical json could be helpful, what is your opinion? Like this example:
{"c0":{"c11": "I am not NULL", "c1": null}, "c1": "I am not NULL",
"c11":null}.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request:
https://github.com/apache/drill/pull/594#discussion_r81617585
--- Diff:
exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java
---
@@ -131,14 +139,33 @@ public void ensureAtLeastOneField(ComplexWriter
writer) {
PathSegment fieldPath = fieldPathList.get(j);
if (emptyStatus.get(j)) {
if (allTextMode) {
- fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
+for (String fieldName : nullableFields) {
+ fieldWriter.varChar(fieldName);
+}
+ } else {
+fieldWriter.varChar(fieldPath.getNameSegment().getPath());
+ }
} else {
- fieldWriter.integer(fieldPath.getNameSegment().getPath());
+ if (checkNullFields(fieldPathList)) {
--- End diff --
Your fix is not targeted to change behavior under non-allTextMode, right?
If this is the case, is this change in "else" needed? If you want to fix
behavior under non-allTextMode as well, then you should also add unit test for
this mode as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r81619047 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -337,6 +364,7 @@ private void writeData(MapWriter map, FieldSelection selection, boolean moveForw continue outside; } +nullableFields.remove(fieldName); --- End diff -- Is this line needed? Since it is a set. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
Github user chunhui-shi commented on a diff in the pull request: https://github.com/apache/drill/pull/594#discussion_r81619869 --- Diff: exec/java-exec/src/main/java/org/apache/drill/exec/vector/complex/fn/JsonReader.java --- @@ -418,6 +447,7 @@ private void writeDataAllText(MapWriter map, FieldSelection selection, boolean m continue outside; } + nullableFields.remove(fieldName); --- End diff -- Is this line needed? Since it is a set. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] drill pull request #594: DRILL-4842: SELECT * on JSON data results in Number...
GitHub user Serhii-Harnyk opened a pull request: https://github.com/apache/drill/pull/594 DRILL-4842: SELECT * on JSON data results in NumberFormatException You can merge this pull request into a Git repository by running: $ git pull https://github.com/Serhii-Harnyk/drill DRILL-4842 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/594.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #594 commit 190a69a65ad8c144b164c2acacf9718a0ecb3768 Author: Serhii-Harnyk Date: 2016-09-08T18:11:37Z DRILL-4842: SELECT * on JSON data results in NumberFormatException --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
