Dan Hecht has posted comments on this change. Change subject: IMPALA-3804: Push per-split filtering into scanners ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/3561/2/be/src/exec/base-sequence-scanner.cc File be/src/exec/base-sequence-scanner.cc: PS2, Line 125: ) nit: extraneous parens http://gerrit.cloudera.org:8080/#/c/3561/2/be/src/exec/hdfs-scan-node.cc File be/src/exec/hdfs-scan-node.cc: Line 244: #endif it would be nice to not have this if-def in the code, or at least around so much code. How about either earlier in the file doing: #ifdef NDEBUG #define FLAGS_skip_file_runtime_filtering false #endif or: static inline bool DebugSkipFileRuntimeFiltering() { #ifdef NDEBUG return true; #else return FLAGS_skip... #endif } and then use that here? http://gerrit.cloudera.org:8080/#/c/3561/2/be/src/exec/hdfs-scanner.cc File be/src/exec/hdfs-scanner.cc: Line 709: return true; rather than doing all this close logic here, it seems more straight forward to just make the scanner derived class do that. After all, CLOSE_ALL and CLOSE_ONLY_THIS_RANGE is only used in one place, and those classes already have to know how to handle their own scan range weirdnesses in other ways. http://gerrit.cloudera.org:8080/#/c/3561/2/be/src/exec/hdfs-scanner.h File be/src/exec/hdfs-scanner.h: PS2, Line 429: artefact never knew about this spelling -- To view, visit http://gerrit.cloudera.org:8080/3561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f92178f642695e0e9ef901373a5e9f2878a78ce Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Henry Robinson <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-HasComments: Yes
