impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5479: Propagate the argument type in RawValue::Compare()
Date Fri, 09 Jun 2017 21:51:37 GMT
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 <kwho@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message