Michael Ho has posted comments on this change. Change subject: IMPALA-5479: Propagate the argument type in RawValue::Compare() ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/codegen/codegen-anyval.cc File be/src/codegen/codegen-anyval.cc: Line 702 > Its a little weird that it can't infer that this is already constant. The n We were using a stack variable. LLVM is hesitant to propagate that as a constant. http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/raw-value-ir.cc File be/src/runtime/raw-value-ir.cc: PS1, Line 29: IR_ALWAYS_INLINE > We don't need the attribute in both declaration and definition do we? Agree that it's not needed but it seems clearer (for documentation purpose) to have it at the definition too. It seems to be the prevalent pattern in our cross-compiled code now. http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter-ir.cc File be/src/runtime/runtime-filter-ir.cc: PS1, Line 24: IR_ALWAYS_INLINE > We don't need the attribute in both declaration and definition do we? Agree that it's not needed but it seems clearer (for documentation purpose) to have it at the definition too. It seems to be the prevalent pattern in our cross-compiled code now. Do you feel strongly that we should clean them up ? http://gerrit.cloudera.org:8080/#/c/7140/1/be/src/runtime/runtime-filter.h File be/src/runtime/runtime-filter.h: Line 63: /// Inlined in IR so that the constant 'col_type' can be propagated. > Maybe something like "Inlined in IR so that the constnt 'col_type' can be p Done -- To view, visit http://gerrit.cloudera.org:8080/7140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I10b5b284e3da03024476a9620a12d6e7fbf08b3c Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
