[Impala-ASF-CR] IMPALA-10851: Codegen for structs

["Michael Smith (Code Review)" <ge...@cloudera.org>]

Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/18526 )

Change subject: IMPALA-10851: Codegen for structs
......................................................................


Patch Set 10:

(7 comments)

I think I understand most of what's going on, but have a few questions and suggestions.

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/codegen/codegen-anyval.h
File be/src/codegen/codegen-anyval.h:

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/codegen/codegen-anyval.h@21
PS10, Line 21: #include "codegen/codegen-anyval-read-write-info.h"
This could be a forward declaration of CodegenAnyValReadWriteInfo rather than including the header.


http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/codegen/codegen-anyval.cc@a669
PS10, Line 669: 
Needs a rebase to resolve adding TYPE_MAP here.


http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/codegen/codegen-anyval.cc@1025
PS10, Line 1025:   // of i8.
This doesn't really explain why we handle booleans as i8 here, but store them as i1.


http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/exec/hash-table.cc@a786
PS10, Line 786: 
Was this essentially NaN? I'm trying to figure out why this store op seems to have disappeared, and what it was doing.


http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/exprs/scalar-expr-evaluator-ir.cc
File be/src/exprs/scalar-expr-evaluator-ir.cc:

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/exprs/scalar-expr-evaluator-ir.cc@134
PS10, Line 134:       if (v.is_null) return nullptr;
Could the 'v.is_null' check be done before the case statement? Looks like AnyVal stores is_null, so the reinterpret_cast shouldn't change how it's checked.


http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/udf/udf-ir.cc
File be/src/udf/udf-ir.cc:

http://gerrit.cloudera.org:8080/#/c/18526/10/be/src/udf/udf-ir.cc@58
PS10, Line 58: uint8_t* FnCtxAllocateForResults(FunctionContext* ctx, int64_t byte_size) {
Where is this used?


http://gerrit.cloudera.org:8080/#/c/18526/10/tests/query_test/test_nested_types.py
File tests/query_test/test_nested_types.py:

http://gerrit.cloudera.org:8080/#/c/18526/10/tests/query_test/test_nested_types.py@143
PS10, Line 143:             'disable_codegen': ['True', 'False']}))
Why reverse the order here? Looking at how create_exec_option_dimension works, I think you could set disable_codegen_rows_threshold and exec_single_node_rows_threshold as part of this dictionary.

Also I'm confused about these tests setting disable_codegen explicitly. These are the defaults for create_exec_option_dimension.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5272c3f095fd9f07877104ee03c8e43d0c4ec0b6
Gerrit-Change-Number: 18526
Gerrit-PatchSet: 10
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <ga...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 28 Sep 2022 22:06:02 +0000
Gerrit-HasComments: Yes