Tim Armstrong has posted comments on this change. Change subject: IMPALA-5031: Remove undefined behavior "reference binding to null" ......................................................................
Patch Set 3: Code-Review+1 (6 comments) Seems good to me aside from a couple of places where the search/replace was overzealous. http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/hash-table.cc File be/src/exec/hash-table.cc: Line 731: // ctx_vector = build_expr_ctxs_.data() / ctx_vector = probe_expr_ctxs_.data() > I'm not sure why this line is here at all. Should we remove it? Looks like it's documenting what code the line below is mean to generate. http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/exec/parquet-column-stats.inline.h File be/src/exec/parquet-column-stats.inline.h: Line 75: v, bytes_needed, reinterpret_cast<uint8_t*>(&(*out)[0])); > Out cannot be empty here, but the code looks suspicious. Should we change i out->data() would be more readable anyway IMO. http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/experiments/tuple-splitter-test.cc File be/src/experiments/tuple-splitter-test.cc: PS3, Line 284: split_counts_ > Will this work if split_counts_ is empty()? Should we have a comment to exp Using sizeof() like this is always valid (it never actually evaluates the expression). Seems ok to me. http://gerrit.cloudera.org:8080/#/c/7008/3/be/src/util/internal-queue-test.cc File be/src/util/internal-queue-test.cc: Line 127: queue.Enqueue(nodes.data()); These aren't necessary because of the resize aboveand inconsistent with using &nodes[1] Line 194: queue.Enqueue(nodes.data()); Revert this one? It's not necessary and inconsistent with the lines immediately below. Line 202: queue.Enqueue(nodes.data()); Same here. -- To view, visit http://gerrit.cloudera.org:8080/7008 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifef5c3de686318bff4eac7dfad6e1639ddadb8f2 Gerrit-PatchSet: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
