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 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h File be/src/exec/delimited-text-parser.h: http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@51 PS1, Line 51: tuple_delim > what should be passed in here when !delimited_tuples? let's at least note Done http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@96 PS1, Line 96: /// This function is disabled for non-sequence file parsing. : template <bool process_escapes, bool enabled=true> : typename std::enable_if<!delimited_tuples && enabled, Status>::type > why do we need that? Not necessary. Initially I thought there was more split functionality in this class than there actually is and I wanted to conditionally get rid of functions that were invalid based on delimited_tuples. I think a DCHECK would be more than sufficient. http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.h@222 PS1, Line 222: tuple_delim_ > how about saying that's valid only when delimited_tuples is true? Done http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h File be/src/exec/delimited-text-parser.inline.h: http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@55 PS1, Line 55: b > why isn't that delimited_tuples? is it something different will fix http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@163 PS1, Line 163: tuple_delim_ || (tuple_delim_ > do those need to be guarded? Subtly, no. Any delimiter of '\0' will not be included in the xmm_delim_search field, so the corresponding bit will never be set. http://gerrit.cloudera.org:8080/#/c/9525/1/be/src/exec/delimited-text-parser.inline.h@235 PS1, Line 235: enabled > what's that about? An extra fake template parameter defaulting to true is used in the class definition to eliminating this function by utilizing SFINAE because it isn't legal to use fail substitution on definitions only depending on a class template parameter. -- 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: 1 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, 08 Mar 2018 21:19:37 +0000 Gerrit-HasComments: Yes
