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<? extends JsonSerializable> 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; } : /** : * Show internal structures such as substitution maps. : */ : public ToJsonOptions showInternals(boolean flag) : { showInternals_ = flag; return this; } : /** : * Create more compact JSON by "eliding" structures: omit fields which : * are null or false, fields at their default values, etc. : */ : public ToJsonOptions elide(boolean flag) : { elide_ = flag; return this; } : /** : * Dedup the output by giving each object an ID, then including just the : * id if the object appears again in the output. The ID is based on tree > This style generally not recommended. Just put each statement in a new line Done. Are the Impala style rules specified anywhere? Each project has different quirks. http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java File fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java: http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/main/java/org/apache/impala/common/serialize/TreeSerializer.java@26 PS2, Line 26: > It's probably better to extend java.lang.AutoCloseable interface instead. T 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@39 PS2, Line 39: : protected abstract void build(); : : protected void verify(String expected) { : builder_.close(); : assertEquals(expected, strWriter_.toString()); : } > I haven't really seen this style much outside C++ or Rust. We can actually The style is common in tests to avoid accidental reuse of common variables from one test to another. Also, I tend to favor a simpler structure in tests to encourage others to add to them. Some folks get discouraged when they have to figure out fancy test code. Did (a variation of) your suggestion. In the end, the code is no smaller, there are more "moving parts" and the code harder to debug. I kind of think that simpler is better for tests. http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@65 PS2, Line 65: > To visualize the output better I think we should properly indent the output Would love to do it that way if Java had something like a "HERE" doc. But, as it is, the Java form is just as hard to follow as this form: "{\n" + " f1: null,\n" + " f2: 10\n" + ... The easiest way to see the output is when tests fail, Eclipse has a very nice diff view: it shows the expected and actual strings. formatted nicely. And, in the next patch or two, you'll see the actual generated JSON which is a much more realistic way to evaluate the generated text. http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@207 PS2, Line 207: build(); > I believe the same style as what I mentioned above can also be used. Done -- 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 <par0...@yahoo.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Comment-Date: Sat, 15 Dec 2018 03:31:17 +0000 Gerrit-HasComments: Yes
