Alex Behm has posted comments on this change. Change subject: IMPALA-3905: Add single-threaded scan node. ......................................................................
Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/4113/3/be/src/exec/hdfs-scan-node-mt.cc File be/src/exec/hdfs-scan-node-mt.cc: PS3, Line 48: !files.second.empty() > I was more wondering how you discovered this and whether we would detect it I discovered this by a "select * from tbl" query against a text table with mt_num_cores > 1 to enable the new scan node. The query should have failed but didn't. Apparently we create an entry in the per_type_files_ for all file formats regardless of whether there are files or not. This check only seems interesting for the intermediate stage where we haven't yet converted all scanners to implement GetNext(). I don't think we can make this scan node the default until then. It may make sense to add a new test for this intermediate stage to put stuff like this into, but let's discuss with Marcel also. Line 72: if (scan_range_ == NULL || scanner_->eos()) { > We don't set scan_range_ to NULL in the ReachedLimit() case, unless I misse Good point, better to set scan_range_ to NULL there, too. Done. -- To view, visit http://gerrit.cloudera.org:8080/4113 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98cc7f970e1575dd83875609985e1877ada3d5e0 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
