Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/15104 )
Change subject: IMPALA-9228: ORC scanner reads rows into scratch batch ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-columnar-scanner.h File be/src/exec/hdfs-columnar-scanner.h: http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-columnar-scanner.h@40 PS4, Line 40: () > nit: "...in the derived classes." Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-columnar-scanner.h@45 PS4, Line 45: > nit: you could add "= nullptr" to make the default value of it obvious. And Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-orc-scanner.h File be/src/exec/hdfs-orc-scanner.h: http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-orc-scanner.h@258 PS4, Line 258: has a collection : /// amongst its children. > nit: sounds weird to me, maybe just "has a collection amongst..." Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/hdfs-orc-scanner.cc@703 PS4, Line 703: DCHECK(scratch_batch_->AtEnd()); > We hit this DCHECK when we come from the loop of AssembleRows(). We can als Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/orc-column-readers.h File be/src/exec/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/orc-column-readers.h@130 PS4, Line 130: to implement static polymorphism via the "curiously > nit: to implement static polymorphism via the "curiously recurring template Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/orc-column-readers.h@161 PS4, Line 161: > If you'd add the 'final' specifier to the ReadValue() implementations then Done http://gerrit.cloudera.org:8080/#/c/15104/4/be/src/exec/orc-column-readers.h@469 PS4, Line 469: /// Keep row index if we're top level readers > You need to rename this to make clang-tidy happy as it hides virtual functi Done -- To view, visit http://gerrit.cloudera.org:8080/15104 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I56db0325dee283d73742ebbae412d19693fac0ca Gerrit-Change-Number: 15104 Gerrit-PatchSet: 5 Gerrit-Owner: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 02 Mar 2020 09:45:55 +0000 Gerrit-HasComments: Yes
