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<? extends JsonSerializable> 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 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 : * structure, so it tends to be consistent across runs. : */ : public ToJsonOptions dedup(boolean flag) { dedup_ = flag; return this; } This style generally not recommended. Just put each statement in a new line. 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: void close(); It's probably better to extend java.lang.AutoCloseable interface instead. That way we can use try/resource. 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 http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@39 PS2, Line 39: { : // Trivial case : StringWriter strWriter = new StringWriter(); : TreeBuilder builder = new TreeBuilder(new PrintWriter(strWriter)); : builder.close(); : assertEquals("", strWriter.toString()); : } I haven't really seen this style much outside C++ or Rust. We can actually simplify this with a bit of functional style. /** * The function takes the takes a builder and returns the expected output string. */ private static void testBuilder(Function<TreeBuilder, String> function) { StringWriter strWriter = new StringWriter(); assertEquals(function.apply(new TreeBuilder(new PrintWriter(strWriter))), strWriter.toString()); } To use it: // Trivial case testBuilder(builder -> { builder.close(); return ""; }); // Trivial root testBuilder(builder -> { assertEquals(1, builder.root()); builder.close(); return "{\n}\n"; }); http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@65 PS2, Line 65: {\n f1: null,\n f2: 10,\n f3: null,\n f4: \"abc\"\n}\n To visualize the output better I think we should properly indent the output by having a new line after \n?. It's a little bit hard to figure out how the output looks like in a single line. http://gerrit.cloudera.org:8080/#/c/12079/2/fe/src/test/java/org/apache/impala/common/serialize/TreeFormatterTest.java@207 PS2, Line 207: public void testSerialize() { I believe the same style as what I mentioned above can also be used. -- 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 <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 14 Dec 2018 21:42:29 +0000 Gerrit-HasComments: Yes
