Norbert Luksa has posted comments on this change. ( http://gerrit.cloudera.org:8080/15051 )
Change subject: IMPALA-9226: Improve string allocations of the ORC scanner ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/15051/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15051/6//COMMIT_MSG@35 PS6, Line 35: Also run TPC-H queries on the TPC-H benchmark where some : queries' runtime improved by around 10-15%, > You could also state explicitly whether other queries have regressed or not Done http://gerrit.cloudera.org:8080/#/c/15051/3/be/src/exec/hdfs-orc-scanner.cc File be/src/exec/hdfs-orc-scanner.cc: http://gerrit.cloudera.org:8080/#/c/15051/3/be/src/exec/hdfs-orc-scanner.cc@683 PS3, Line 683: > It is still used for CollectionValueBuilder. Oh, right. http://gerrit.cloudera.org:8080/#/c/15051/6/be/src/exec/orc-column-readers.h File be/src/exec/orc-column-readers.h: http://gerrit.cloudera.org:8080/#/c/15051/6/be/src/exec/orc-column-readers.h@202 PS6, Line 202: batch_ = static_cast<orc::StringVectorBatch*>(orc_batch); : if (orc_batch == nullptr) return Status::OK(); > Is it ok that batch_ is not set to null if orc_batch is null? Swapped the two lines. http://gerrit.cloudera.org:8080/#/c/15051/6/be/src/exec/orc-column-readers.h@210 PS6, Line 210: &batch_- > Does Orc support mixed encoding, e.g. a stripe that has both dictionary and Yes, according to the ORC documentation it cannot change through stripes. I'm not sure if it's worth to add an extra variable just to make sure that this behaviour is preserved by the ORC lib. There are DCHECKs which should also fail in that case (dynamic casting to the Vector batch is based on the encoding). I think a comment to clarify this behaviour would be enough, but please raise your concern if you think otherwise. http://gerrit.cloudera.org:8080/#/c/15051/6/be/src/exec/orc-column-readers.h@211 PS6, Line 211: > "RETURN_IF_ERROR" + "return Status::OK()" could be simplified to "return". Done -- To view, visit http://gerrit.cloudera.org:8080/15051 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If2d975946fb6f4104d8dc98895285b3a0c6bef7f Gerrit-Change-Number: 15051 Gerrit-PatchSet: 7 Gerrit-Owner: Norbert Luksa <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Norbert Luksa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 30 Jan 2020 17:17:10 +0000 Gerrit-HasComments: Yes
