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 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h File be/src/exec/delimited-text-parser.h: http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@174 PS3, Line 174: field_delim_ != tuple_delim_ > is it allowed to have field_delim_ same as tuple_delim_? what's the behavio It is not allowed to have tables created like this by the frontend. It is possible, although unlikely that tables like this already exist. I encountered this behavior in the test, when the default arguments for the constructor created such a parser, so it is tested. I thought the sanest behavior was to only recognize a field delimiter when it was not equal to a tuple delimiter, and tuple delimiters were enabled. Before this, the behavior was nuts, it would not recognize tuples, and instead treat the entire row as an infinite set of columns. http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.h@175 PS3, Line 175: ) > this would be a little easier to read without the extraneous parens around Oh yes, it is just a bunch of OR clauses; we can remove these. http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc File be/src/exec/delimited-text-parser.cc: http://gerrit.cloudera.org:8080/#/c/9525/3/be/src/exec/delimited-text-parser.cc@182 PS3, Line 182: if (DELIMITED_TUPLES) unfinished_tuple_ = false; > can we even get here if !DELIMITED_TUPLES? If not, maybe DCHECK(DELIMINTED Nope; I was just eliding all references to unfinished_tuple_ in the class variant that had DELIMITED_TUPLES as false. I would hope in this case the compiler can help out by proving that new_tuple is always false, but we can help it out by coding it explicitly if you think that would be better. -- 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: 3 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: Wed, 21 Mar 2018 17:00:19 +0000 Gerrit-HasComments: Yes
