[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Abandoned Was meant to help with testing, but moving away from that effort. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 8: Hi Bharath, Fredy gave this a +1. Can you take a quick look and give it the final +2 if it looks good to you? -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 23 Jan 2019 21:03:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 8: Code-Review+1 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 04 Jan 2019 17:01:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 8: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1694/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sun, 30 Dec 2018 22:09:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#8). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework uses Jackson streaming API to JSON. Though the primary purpose is to generate JSON for in PlannerTest text comparisons, the use of Jackson ensures that the JSON could, if wanted, be exported to other tools. While Jackson provides low-level serialization, the classes here provide a set of higher-level abstractions that make the AST serialization (not in this patch) simpler than it would be using the low-level Jackson tools. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeJsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/TreeJsonStringSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java 10 files changed, 1,098 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/8 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 8 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 7: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1685/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 7 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 27 Dec 2018 22:58:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#7). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework uses Jackson streaming API to JSON. Though the primary purpose is to generate JSON for in PlannerTest text comparisons, the use of Jackson ensures that the JSON could, if wanted, be exported to other tools. While Jackson provides low-level serialization, the classes here provide a set of higher-level abstractions that make the AST serialization (not in this patch) simpler than it would be using the low-level Jackson tools. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeJsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/TreeJsonStringSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java 10 files changed, 1,081 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/7 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 7 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1683/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 27 Dec 2018 22:02:34 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 6: (2 comments) http://gerrit.cloudera.org:8080/#/c/12079/6/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/6/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java@204 PS6, Line 204: _objectFieldValueSeparatorWithSpaces = DEFAULT_SEPARATORS.getObjectFieldValueSeparator() + " "; line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/12079/6/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java File fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java: http://gerrit.cloudera.org:8080/#/c/12079/6/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java@22 PS6, Line 22: import org.apache.impala.common.serialize.JacksonTreeSerializer.JacksonTreeStringSerializer; line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Thu, 27 Dec 2018 21:27:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#6). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework uses Jackson streaming API to JSON. Though the primary purpose is to generate JSON for in PlannerTest text comparisons, the use of Jackson ensures that the JSON could, if wanted, be exported to other tools. While Jackson provides low-level serialization, the classes here provide a set of higher-level abstractions that make the AST serialization (not in this patch) simpler than it would be using the low-level Jackson tools. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java 9 files changed, 1,062 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/6 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 6 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 5:
(5 comments)
Thanks for the reviews; addressed comments.
After testing, turns out we can get the Jackson serializer to do pretty much
what is wanted, so have removed the bespoke serializer to keep things simple.
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@83
PS3, Line 83: objectList
> shouldn't this be stringList?
Done
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@88
PS3, Line 88: objectList
> should this be scalarList?
Done
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@34
PS3, Line 34: public ToJsonOptions showSource(boolean flag)
: { showSource_ = flag; return this; }
> Sorry if I wasn't clear. I meant to use the standard style.
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java
File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@32
PS2, Line 32:
> nit: remove new line
This file is now removed, but made the same fix in the new Jackson-based
version.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@39
PS2, Line 39:
:
:
:
:
:
:
> What do you think about this?
Handy trick I'll use in the future. For this one, the class approach allowed a
very simple port from the original bespoke implementation to the Jackson
implementation which Phil suggested. I'd have to do quite a bit of rewrite to
adopt the above version. The above version has more redundancy than the current
revised version.
Please take a look. Happy to do the rewrite if necessary, but let's make that
decision based on the current code.
--
To view, visit http://gerrit.cloudera.org:8080/12079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c
Gerrit-Change-Number: 12079
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Thu, 20 Dec 2018 02:26:18 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 5: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1643/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 19 Dec 2018 01:34:18 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/12079/5/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/5/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java@204 PS5, Line 204: _objectFieldValueSeparatorWithSpaces = DEFAULT_SEPARATORS.getObjectFieldValueSeparator() + " "; line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/12079/5/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java File fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java: http://gerrit.cloudera.org:8080/#/c/12079/5/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java@22 PS5, Line 22: import org.apache.impala.common.serialize.JacksonTreeSerializer.JacksonTreeStringSerializer; line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Wed, 19 Dec 2018 00:51:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#5). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework uses Jackson streaming API to JSON. Though the primary purpose is to generate JSON for in PlannerTest text comparisons, the use of Jackson ensures that the JSON could, if wanted, be exported to other tools. While Jackson provides low-level serialization, the classes here provide a set of higher-level abstractions that make the AST serialization (not in this patch) simpler than it would be using the low-level Jackson tools. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java 9 files changed, 1,062 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/5 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: > > To what extent is this the Jackson streaming API? > > The Jackson Streaming API is low level, while the serialization API > provided here operates at a higher level, making assumptions about > the kinds of data we want to serialize. > > Jackson is very good for producing standard JSON. > > To show the relationship, went ahead and implemented a serializer > based on Jackson. The code is actually longer than the bespoke > version, but the Jackson version produces standard JSON we can > export to other tools. Tell me more about what you're trying to do? If we're going to call it JSON, it should probably be parseable with most JSON parsers. Various Hadoop ecosystem things use plenty of Jackson and class annotations and what-not. It's not perfect, but it seems pretty standard. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 04:16:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@83
PS3, Line 83: objectList
shouldn't this be stringList?
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@88
PS3, Line 88: objectList
should this be scalarList?
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@34
PS3, Line 34: public ToJsonOptions showSource(boolean flag)
: { showSource_ = flag; return this; }
Sorry if I wasn't clear. I meant to use the standard style.
public ToJsonOptions showSource(boolean flag) {
showSource_ = flag;
return this;
}
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java
File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@39
PS2, Line 39:
: protected abstract void build();
:
: protected void verify(String expected) {
: builder_.close();
: assertEquals(expected, strWriter_.toString());
: }
> The style is common in tests to avoid accidental reuse of common variables
What do you think about this?
@FunctionalInterface
private interface Verifier {
void verify(String expected);
}
private static Verifier testBuilder(Consumer consumer) {
StringWriter strWriter = new StringWriter();
TreeBuilder builder = new TreeBuilder(new PrintWriter(strWriter));
consumer.accept(builder);
return (expected) -> {
builder.close();
assertEquals(expected, strWriter.toString());
};
}
public static Verifier testFormatter(ToJsonOptions options,
Consumer consumer) {
JsonTreeFormatter fmt = new JsonTreeFormatter(options);
fmt.build();
consumer.accept(fmt);
return (expected) -> {
assertEquals(expected, fmt.toString());
};
}
public static Verifier testFormatter(Consumer consumer) {
return testFormatter(ToJsonOptions.full(), consumer);
}
And we can use it like this:
testBuilder(builder -> {
assertEquals(1, builder.root());
}).verify("{\n}\n");
testFormatter(fmt -> {
ObjectSerializer os = fmt.root();
os.field("bool", true);
}).verify("{\n bool: true\n}\n");
IMO lambda is generally more readable for creating a DSL (and generates better
code because of invokedynamic, although I don't think it matters for this unit
test) than using anonymous inner class.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@65
PS2, Line 65:
> Would love to do it that way if Java had something like a "HERE" doc. But,
I'm too pampered with IntelliJ that can nicely preserve the formatting for
String multi-line :) Anyway, I'm okay with this.
--
To view, visit http://gerrit.cloudera.org:8080/12079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c
Gerrit-Change-Number: 12079
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Paul Rogers
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Sat, 15 Dec 2018 04:15:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1623/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 04:15:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: > To what extent is this the Jackson streaming API? The Jackson Streaming API is low level, while the serialization API provided here operates at a higher level, making assumptions about the kinds of data we want to serialize. Jackson is very good for producing standard JSON. To show the relationship, went ahead and implemented a serializer based on Jackson. The code is actually longer than the bespoke version, but the Jackson version produces standard JSON we can export to other tools. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 03:36:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 4: (2 comments) http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java@204 PS4, Line 204: _objectFieldValueSeparatorWithSpaces = DEFAULT_SEPARATORS.getObjectFieldValueSeparator() + " "; line too long (101 > 90) http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java File fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java: http://gerrit.cloudera.org:8080/#/c/12079/4/fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java@22 PS4, Line 22: import org.apache.impala.common.serialize.JacksonTreeSerializer.JacksonTreeStringSerializer; line too long (92 > 90) -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 03:29:50 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 3:
(19 comments)
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java
File
fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@26
PS2, Line 26: /**
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@28
PS2, Line 28:*/
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@51
PS2, Line 51: public void object(String name, JsonSerializable obj) {
: if (obj != null) obj.serialize(this);
: else if (!options().elide()) scalar(name, null);
: }
> nit:
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@35
PS2, Line 35: /**
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@45
PS2, Line 45: }
> I may miss something obvious, but I don't see anything that requires suppre
JSON-Simple is old school: it's objects are un-parameterized Maps, which scream
warnings when used. Added comment to explain this fact.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@66
PS2, Line 66: return serializer_.options();
> Why is this method empty?
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@74
PS2, Line 74:
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@145
PS2, Line 145: obj.serialize(object());
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@151
PS2, Line 151: p
> nit: extra space
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@46
PS2, Line 46: * by the order that objects are emitted, and thus is consistent
across runs.
> nit: remove new line
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@58
PS2, Line 58: /**
:* Object serializer that generates formatted JSON output.
> If the if condition doesn't fit into a single line, we should add {}.
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@75
PS2, Line 75: void objectList(String name, List
objs);
> I think it's better to not use shorthand name. We don't save that many cha
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@80
PS2, Line 80: stringL
> similarly here, we can call it stringList instead.
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@85
PS2, Line 85: stringL
> same as above
Done
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@32
PS2, Line 32:* Include the source SQL.
:*/
: public ToJsonOptions showSource(boolean flag)
: { showSource_ = flag; return this; }
: /**
:* Include details of the structures created by analyais, such
:* as the output result set.
:*/
: public ToJsonOptions showOutput(boolean flag)
: { showOutput_ = flag; return this; }
: /**
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#4). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with three implementations: * Jackson streaming API to produce standard JSON for export to external systems. * Bespoke JSON serializer optimized to produce compact, repeatable output for testing. This form may be adapted to produce YAML, which is more compact than JSON in order to reduce test file size. * JSONSimple (already an Impala dependency) to create JSON objects. Not super useful, but shows how to use other object-based JSON frameworks if needed. This framework itself is independent of the AST and can also be used to serialize the physical plan as well. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JacksonTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/SerializationError.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/JacksonTreeSerializerTest.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 12 files changed, 1,944 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/4 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1622/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Sat, 15 Dec 2018 01:49:20 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java
File fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java:
http://gerrit.cloudera.org:8080/#/c/12079/3/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@108
PS3, Line 108: }.verify("{\n f_1: null,\n \"f 2\": 10,\n \"f-3\":
null,\n \"f.4\": \"abc\"\n}\n");
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12079
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c
Gerrit-Change-Number: 12079
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger
Gerrit-Comment-Date: Sat, 15 Dec 2018 01:07:03 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Fredy Wijaya, Philip Zeyliger, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#3). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with two implementations: one that uses JSONSimple (already an Impala dependency) to create JSON objects, another that uses a bespoke JSON serializer optimized to produce compact, repeatable output for testing. The framework allows other serializers to be added later. Perhaps YAML might prove even more compact than JSON. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 9 files changed, 1,283 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/3 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 3 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 2: To what extent is this the Jackson streaming API? https://github.com/FasterXML/jackson-docs/wiki/JacksonStreamingApi -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Philip Zeyliger Gerrit-Comment-Date: Fri, 14 Dec 2018 22:43:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 2:
(21 comments)
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java
File
fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@26
PS2, Line 26:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@28
PS2, Line 28:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java@51
PS2, Line 51: if (obj != null)
: obj.serialize(this);
: else if (!options().elide())
: scalar(name, null);
nit:
if (obj != null) obj.serialize(this);
else if (!options().elide()) scalar(name, null);
Braces can be optional if it's in a single line.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@35
PS2, Line 35:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@45
PS2, Line 45: @SuppressWarnings("unchecked")
I may miss something obvious, but I don't see anything that requires
suppressing a warning here and anywhere else in this file.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@66
PS2, Line 66: // TODO Auto-generated method stub
Why is this method empty?
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@74
PS2, Line 74:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@145
PS2, Line 145:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java@151
PS2, Line 151:
nit: extra space
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@46
PS2, Line 46:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@58
PS2, Line 58: if (value != null || !options().elide())
: formatter_.builder_.quotedField(level_, name, value);
If the if condition doesn't fit into a single line, we should add {}.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java
File fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@75
PS2, Line 75: void objList(String name, List
objs);
I think it's better to not use shorthand name. We don't save that many chars
anyway. We can call it objectList.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@80
PS2, Line 80: strList
similarly here, we can call it stringList instead.
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java@85
PS2, Line 85: strList
same as above
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java
File fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java:
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@25
PS2, Line 25:
nit: remove new line
http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java@32
PS2, Line 32: /**
:* Include the source SQL.
:*/
: public ToJsonOptions showSource(boolean flag) { showSource_ =
flag; return this; }
: /**
:* Include details of the structures created by analyais, such
:* as the output result set.
:*/
: public ToJsonOptions showOutput(boolean flag) { showOutput_ =
flag; return
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1614/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 20:51:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12079 to look at the new patch set (#2). Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with two implementations: one that uses JSONSimple (already an Impala dependency) to create JSON objects, another that uses a bespoke JSON serializer optimized to produce compact, repeatable output for testing. The framework allows other serializers to be added later. Perhaps YAML might prove even more compact than JSON. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 9 files changed, 1,199 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/2 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12079 ) Change subject: IMPALA-7968, Part 1: JSON serialization framework .. Patch Set 1: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1603/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Fri, 14 Dec 2018 01:39:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Impala Public Jenkins has posted comments on this change. (
http://gerrit.cloudera.org:8080/12079 )
Change subject: IMPALA-7968, Part 1: JSON serialization framework
..
Patch Set 1:
(183 comments)
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java
File fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java:
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@47
PS1, Line 47: private static class ObjectFormatter extends
AbstractObjectSerializer {
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@48
PS1, Line 48: private final JsonTreeFormatter formatter_;
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@49
PS1, Line 49: private int level_;
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@51
PS1, Line 51: public ObjectFormatter(JsonTreeFormatter formatter, int
level) {
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@52
PS1, Line 52: formatter_ = formatter;
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@53
PS1, Line 53: level_ = level;
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@54
PS1, Line 54: }
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@56
PS1, Line 56: @Override
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@57
PS1, Line 57: public void field(String name, String value) {
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@60
PS1, Line 60: }
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@62
PS1, Line 62: @Override
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@64
PS1, Line 64: formatter_.builder_.unquotedField(level_, name,
value);
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@65
PS1, Line 65: }
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@67
PS1, Line 67: @Override
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@68
PS1, Line 68: public ObjectSerializer object(String name) {
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@69
PS1, Line 69: return new ObjectFormatter(formatter_,
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@70
PS1, Line 70:
formatter_.builder_.objectField(level_, name));
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@71
PS1, Line 71: }
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@73
PS1, Line 73: @Override
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@74
PS1, Line 74: public ToJsonOptions options() {
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@75
PS1, Line 75: return formatter_.options();
tab used for whitespace
http://gerrit.cloudera.org:8080/#/c/12079/1/fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java@76
PS1, Line 76: }
tab used for whitespace
[Impala-ASF-CR] IMPALA-7968, Part 1: JSON serialization framework
Paul Rogers has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12079 Change subject: IMPALA-7968, Part 1: JSON serialization framework .. IMPALA-7968, Part 1: JSON serialization framework Provides a set of interfaces and classes to serialize the analyzer AST to JSON. This part provides the serialization framework itself, later parts will provide the serialization code and test framework. The framework is modular with two implementations: one that uses JSONSimple (already an Impala dependency) to create JSON objects, another that uses a bespoke JSON serializer optimized to produce compact, repeatable output for testing. The framework allows other serializers to be added later. Perhaps YAML might prove even more compact than JSON. Testing: test cases exercise the mechanisms. The code is an island in the patch; no other code uses it yet. Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c --- A fe/src/main/java/org/apache/impala/common/serialize/AbstractTreeSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ArraySerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializable.java A fe/src/main/java/org/apache/impala/common/serialize/JsonSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/JsonTreeFormatter.java A fe/src/main/java/org/apache/impala/common/serialize/ObjectSerializer.java A fe/src/main/java/org/apache/impala/common/serialize/ToJsonOptions.java A fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java A fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java 9 files changed, 1,197 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/79/12079/1 -- To view, visit http://gerrit.cloudera.org:8080/12079 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie3101c2708bf6cf4bec61af83a5db9b6d70ddd9c Gerrit-Change-Number: 12079 Gerrit-PatchSet: 1 Gerrit-Owner: Paul Rogers
