[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-02-07 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9063 )

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@263
PS1, Line 263: GetType
> Nearly all calls to this functions use the ::LLVM_CLASS_NAME constant of st
I'm not sure that the templated version would improve readability enough to 
offset the additional noise in compiler error messages (although I could be 
wrong :)).


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@497
PS1, Line 497:   llvm::Type* i8_type() { return 
GetInternalRepresentation(TYPE_TINYINT); }
> Calling GetSlotType() is somewhat shorter, but I can switch to llvm calls i
Changing the interface first makes sense, we should switch over the 
implementations in the final patch though.



--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 08 Feb 2018 01:17:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-23 Thread Csaba Ringhofer (Code Review) 
Hello Lars Volker, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9063

to look at the new patch set (#3).

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..

IMPALA-5801: [draft] Clean up codegen GetType() interface

First draft - my goal with this patch is create a review
where we can decide on the scope of the refactor and
agree on the names of the functions.

Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/text-converter.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/tuple.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
21 files changed, 224 insertions(+), 210 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/9063/3
--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-23 Thread Csaba Ringhofer (Code Review) 
Hello Lars Volker, Tim Armstrong,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/9063

to look at the new patch set (#2).

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..

IMPALA-5801: [draft] Clean up codegen GetType() interface

First draft - my goal with this patch is create a review
where we can decide on the scope of the refactor and
agree on the names of the functions.

Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/text-converter.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/tuple.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
21 files changed, 224 insertions(+), 210 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/9063/2
--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-22 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9063 )

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@253
PS1, Line 253: GetInternalRepresentation
Maybe GetSlotType() since it's how the slot within a Tuple is represented? We 
use that terminology in some other places.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@263
PS1, Line 263: GetType
I wonder if we should rename this to GetNamedType() or GetTypeFromModule() or 
something along those lines?


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@496
PS1, Line 496:   llvm::Type* boolean_type() { return 
GetInternalRepresentation(TYPE_BOOLEAN); }
> Maybe bool_type would be more logical in a c++ context.
Makes sense to me.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@497
PS1, Line 497:   llvm::Type* i8_type() { return 
GetInternalRepresentation(TYPE_TINYINT); }
Any reason not to call the LLVM functions directly, like i128_type() i.e. 
llvm::Type::getInt8Ty(context());


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@497
PS1, Line 497:   llvm::Type* i8_type() { return 
GetInternalRepresentation(TYPE_TINYINT); }
 :   llvm::Type* i16_type() { return 
GetInternalRepresentation(TYPE_SMALLINT); }
 :   llvm::Type* i32_type() { return 
GetInternalRepresentation(TYPE_INT); }
 :   llvm::Type* i64_type() { return 
GetInternalRepresentation(TYPE_BIGINT); }
> I have chosen i8_type() instead of int8_type(), because i128_type() already
The short type names seem good.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@524
PS1, Line 524: I8Constant
I think GetI8Constant() would be more consistent with other name (although more 
verbose unfortunately).


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@525
PS1, Line 525: return GetIntConstant(TYPE_TINYINT, val);
We could also just call the LLVM function directly here.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/exprs/slot-ref.cc
File be/src/exprs/slot-ref.cc:

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/exprs/slot-ref.cc@191
PS1, Line 191:   llvm::Value* tuple_offset = 
llvm::ConstantInt::get(codegen->i32_type(), tuple_idx_);
I feel like this is more readable, thank you.



--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 22 Jan 2018 22:05:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-18 Thread Csaba Ringhofer (Code Review) 
Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/9063 )

Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..


Patch Set 1:

(3 comments)

The declarations of the new/renamed functions are in llvm-codegen.h.

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@256
PS1, Line 256:   llvm::PointerType* GetInternalRepresentationPtr(const 
ColumnType& type);
I really do not like this name, if anyone has a better idea, please share it.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@496
PS1, Line 496:   llvm::Type* boolean_type() { return 
GetInternalRepresentation(TYPE_BOOLEAN); }
Maybe bool_type would be more logical in a c++ context.


http://gerrit.cloudera.org:8080/#/c/9063/1/be/src/codegen/llvm-codegen.h@497
PS1, Line 497:   llvm::Type* i8_type() { return 
GetInternalRepresentation(TYPE_TINYINT); }
 :   llvm::Type* i16_type() { return 
GetInternalRepresentation(TYPE_SMALLINT); }
 :   llvm::Type* i32_type() { return 
GetInternalRepresentation(TYPE_INT); }
 :   llvm::Type* i64_type() { return 
GetInternalRepresentation(TYPE_BIGINT); }
I have chosen i8_type() instead of int8_type(), because i128_type() already 
used this convention. If we want these names to be short, the type can be 
abbreviated too, for example as int8_t().



--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Comment-Date: Thu, 18 Jan 2018 21:15:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5801: [draft] Clean up codegen GetType() interface

2018-01-18 Thread Csaba Ringhofer (Code Review) 
Csaba Ringhofer has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/9063


Change subject: IMPALA-5801: [draft] Clean up codegen GetType() interface
..

IMPALA-5801: [draft] Clean up codegen GetType() interface

First draft - my goal with this patch is create a review
where we can decide on the scope of the refactor and
agree on the names of the functions.

Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/codegen-anyval.cc
M be/src/codegen/llvm-codegen-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/exec-node.cc
M be/src/exec/filter-context.cc
M be/src/exec/hash-table.cc
M be/src/exec/hdfs-avro-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/partitioned-aggregation-node.cc
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/text-converter.cc
M be/src/exprs/compound-predicates.cc
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/slot-ref.cc
M be/src/runtime/descriptors.cc
M be/src/runtime/tuple.cc
M be/src/runtime/types.cc
M be/src/util/tuple-row-compare.cc
21 files changed, 211 insertions(+), 178 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/9063/1
--
To view, visit http://gerrit.cloudera.org:8080/9063
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib146ca21af82023b0b460f813eae3435b4b2eb77
Gerrit-Change-Number: 9063
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer