Tim Armstrong has posted comments on this change. Change subject: IMPALA-4098: Open()/Close() partition exprs once per fragment instance. ......................................................................
Patch Set 1: Code-Review+1 (6 comments) This is a lot simpler. http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.cc File be/src/runtime/descriptors.cc: Line 216: for (const auto& entry: tdesc.hdfsTable.partitions) { clang-format puts a space before :. Line 512: for (const auto& part_entry: hdfs_tbl->partition_descriptors()) { space before : to avoid later reformatting by clang-format (the way you wrote it is my personal preference but no point arguing with the tool :)) http://gerrit.cloudera.org:8080/#/c/4340/1/be/src/runtime/descriptors.h File be/src/runtime/descriptors.h: PS1, Line 239: threads ... after the descriptor table is opened ... Line 260: std::vector<ExprContext*> partition_key_value_ctxs_; This was already like this but it feels a little wonky to have non-immutable stateful things in the descriptor table. I think your change makes it a lot more explicit, which is good. I suppose there's no other natural place to store per-partition state. PS1, Line 436: Open Maybe PrepareAndOpenExprs()? I think we want to limit the scope of what we do in here, because mostly the descriptor table should just have static metadata. Also that way it won't look like you forgot to Prepare() this thing if you're just looking at the callsites. PS1, Line 439: Close CloseExprs()? -- To view, visit http://gerrit.cloudera.org:8080/4340 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id179df645e500530f4418988f6ce64a03d669892 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
