Zach Amsden has posted comments on this change. ( http://gerrit.cloudera.org:8080/9525 )
Change subject: IMPALA-6389: Make '\0' delimited text files work ...................................................................... Patch Set 2: (5 comments) http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc File be/src/exec/delimited-text-parser-test.cc: http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser-test.cc@160 PS2, Line 160: TupleDelimitedTextParser nul_delim_parser(NUM_COLS, 0, is_materialized_col, NUL_DELIM); > should we also test nul tuple delim with a column delim? if not, why is tha Will add a test for this. http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h File be/src/exec/delimited-text-parser.h: http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@22 PS2, Line 22: #include <type_traits> Will delete this http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@41 PS2, Line 41: thme > them Done http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@55 PS2, Line 55: char field_delim_ = '\0', char collection_item_delim = '^', : char escape_char = '\0'); > do these default argument values still have special meanings? can the delim The only place these have interesting meanings is the unit test. I don't think the defaults are very useful, but I also don't think removing them and adding a bunch of extra unexplained fields to the unit test is helpful either. http://gerrit.cloudera.org:8080/#/c/9525/2/be/src/exec/delimited-text-parser.h@171 PS2, Line 171: bool IsFieldOrCollectionItemDelimiter(char c) { > comment for this -- it's surprisingly non-trivial to understand what this i Will add a comment. -- To view, visit http://gerrit.cloudera.org:8080/9525 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b6f38cbe3f1036f60efd31a31d82d0cd8f3d2a8 Gerrit-Change-Number: 9525 Gerrit-PatchSet: 2 Gerrit-Owner: Zach Amsden <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zach Amsden <[email protected]> Gerrit-Comment-Date: Thu, 15 Mar 2018 18:17:56 +0000 Gerrit-HasComments: Yes
