Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9793 )
Change subject: IMPALA-5384, part 1: introduce InsertExecState ...................................................................... Patch Set 4: (13 comments) http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/coordinator.h@336 PS4, Line 336: /// Returns request's finalize params, or nullptr if not present. > It seems worth documenting that nullptr means that finalization is not requ Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/coordinator.cc@a492 PS4, Line 492: > I reviewed the moved code by copying and pasting the moved parts into gvimd Thanks for double checking. Yeah, I ended up regenerating the patch from the current code to make sure we wouldn't drop any fixes. http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/coordinator.cc@497 PS4, Line 497: HdfsTableDescriptor* hdfs_table; > This isn't a huge deal, but any reason we can't move the table descriptor c Then we'd have to pass in the desc_tbl anyway, right? So, I'd prefer to just pass in the table descriptor. LMK if you feel strongly. http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h File be/src/runtime/insert-exec-state.h: http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@28 PS4, Line 28: #include "common/hdfs.h" > nit: hdfs goes before status Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@41 PS4, Line 41: /// InsertExecState manages the state related to the execution of an Insert > The lifecycle of this is a bit unclear, especially since we construct the o Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@47 PS4, Line 47: class InsertExecState { > The class name and comment are misleading in that this is also used for upd I renamed this class, but stopped short of trying to restructure the thrift representation (or renaming that) since that's a larger thing (we already have a TODO for that). http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@53 PS4, Line 53: typedef std::map<std::string, TInsertPartitionStatus> PartitionStatusMap; > It might be worth extending the comment here and for FileMoveMap to explain This code only moved, but added the comment as suggested (didn't investigate myself). http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@75 PS4, Line 75: void AddKuduPartition(const std::string& name, int64_t id); > The Kudu Partition API is confusing - it only creates a dummy partition wit Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@80 PS4, Line 80: num_modfied_rows > typo: modified Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.h@118 PS4, Line 118: PartitionStatusMap per_partition_status_; > It looks like the contents of these maps is documented in the typedef above The typedefs don't need to be public, so moved them and the comments to be adjacent to the fields. http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.cc File be/src/runtime/insert-exec-state.cc: http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.cc@121 PS4, Line 121: max_ts = std::max(max_ts, > Not the end of the world, but I prefer max<uint64_t>(max_tx, entry.second.k Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/insert-exec-state.cc@431 PS4, Line 431: const std::string& name, int64_t id, const std::string* base_dir) { > unnecessary std:: prefixes in .cc here and below Done http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/runtime-state.h File be/src/runtime/runtime-state.h: http://gerrit.cloudera.org:8080/#/c/9793/4/be/src/runtime/runtime-state.h@327 PS4, Line 327: /// Execution state for INSERT statements. > Also for UPDATE and UPSERT, right? Done -- To view, visit http://gerrit.cloudera.org:8080/9793 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id4c025917620a7bff2acbeb46464f107ab4b7565 Gerrit-Change-Number: 9793 Gerrit-PatchSet: 4 Gerrit-Owner: Dan Hecht <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 28 Mar 2018 00:53:04 +0000 Gerrit-HasComments: Yes
