[Impala-ASF-CR] Add nested testdata flattener

["Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>]

Taras Bobrovytsky has posted comments on this change.

Change subject: Add nested testdata flattener
......................................................................


Patch Set 1:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/5787/1//COMMIT_MSG
Commit Message:

Line 22: 
> How was this tested?
Done


http://gerrit.cloudera.org:8080/#/c/5787/1/testdata/TableFlattener/README
File testdata/TableFlattener/README:

Line 3: 
> It would be useful to explain also how nested data types (array, struct, ma
Added a brief description.


Line 11: 
> Mention that testdata/bin/generate-load-nested.sh uses this.
Done


PS1, Line 12: There are various options to specify the type of input file but the output is always
            : parquet/snappy.
> You could mention that the tool tries to use file extension to infer the in
Done


http://gerrit.cloudera.org:8080/#/c/5787/1/testdata/TableFlattener/pom.xml
File testdata/TableFlattener/pom.xml:

Line 1: <?xml version="1.0" encoding="UTF-8"?>
> Would Maven permit this whole tree be moved to testdata/src/main/java/org/a
Possibly, but I think that should be done after this is checked in.


http://gerrit.cloudera.org:8080/#/c/5787/1/testdata/TableFlattener/src/main/java/org/apache/impala/infra/tableflattener/FlattenedSchema.java
File testdata/TableFlattener/src/main/java/org/apache/impala/infra/tableflattener/FlattenedSchema.java:

PS1, Line 93: "value"
> Why not item for array?
This should not be modified because the query generator relies on this.


PS1, Line 96: "idx"
> Why not pos?
As mentioned above, the query generator relies on these the way there are now. A future patch can modify both this and the query generator.


PS1, Line 103: "_values";
> This will lead to "foo__values" (with 2 underscores). Is that intended?
Yes, this is intended, the query generator relies on this. This could be modified as together with the query generator in the future.


PS1, Line 120: "_"
> Shouldn't this use getNameSeparator() ?
Done


http://gerrit.cloudera.org:8080/#/c/5787/1/testdata/TableFlattener/src/main/java/org/apache/impala/infra/tableflattener/SchemaFlattener.java
File testdata/TableFlattener/src/main/java/org/apache/impala/infra/tableflattener/SchemaFlattener.java:

PS1, Line 51: dataset
> It would read a bit clearer to call this dstDataSet.
Done


Line 60:     Preconditions.checkState(srcSchema.getType() == Type.RECORD);
> It would be nice to have a comment on this method describing what it does a
Let's do this as part of a separate patch. I would like to just get this code in because it works.


PS1, Line 65: field.schema()
> Why not fieldSchema?
Done


Line 85:     // Ensure that the parent schema has an id field so the child can reference the
> Same with this method: add brief description of method and parameter meanin
Let's do this in a separate patch.


PS1, Line 93:     LinkedList<Field> fields = new LinkedList<>();
> Is it correct to be instantiating fields this way, or should you be using t
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/5787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e7a8e53ada9274759a3e2128b97bec292c129c6
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes