You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Daniel Becker (Code Review)" <ge...@cloudera.org> on 2023/11/02 13:08:54 UTC
[Impala-ASF-CR] IMPALA-12373: Small String Optimization for StringValue
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20496 )
Change subject: IMPALA-12373: Small String Optimization for StringValue
......................................................................
Patch Set 8:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/codegen/codegen-anyval.cc@1094
PS8, Line 1094: CollectionVals
Writing "Arrays and maps" would be more informative.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/codegen/codegen-anyval.cc@1094
PS8, Line 1094: has
Nit: 'have'.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/codegen/codegen-anyval.cc@1100
PS8, Line 1100: if (type.type == TYPE_CHAR) {
: len = codegen->GetI32Constant(type.len);
: }
This branch is no longer needed, string types are now handled separately.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/exec/orc/orc-column-readers.cc
File be/src/exec/orc/orc-column-readers.cc:
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/exec/orc/orc-column-readers.cc@299
PS8, Line 299: src_len > dst_len
Optional: I find "dst_len < src_len" easier to read now, because on the next line we set 'dst_len'.
http://gerrit.cloudera.org:8080/#/c/20496/7/be/src/exec/parquet/parquet-common.h
File be/src/exec/parquet/parquet-common.h:
http://gerrit.cloudera.org:8080/#/c/20496/7/be/src/exec/parquet/parquet-common.h@577
PS7, Line 577: fixed_len_size < str_len
> If fixed_len_size is greater than 0 then it still caps the length of 'v' or
I think I missed something, it should be correct.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/buffered-tuple-stream-test.cc
File be/src/runtime/buffered-tuple-stream-test.cc:
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/buffered-tuple-stream-test.cc@1985
PS8, Line 1985: sv->Smallify()
I'd put this on its own line because it has a side effect. It would be easier to read that way.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/sorter.cc
File be/src/runtime/sorter.cc:
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/sorter.cc@737
PS8, Line 737: auto
Writing out the actual type (SlotDescriptor*) would improve readability.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/sorter.cc@738
PS8, Line 738: bool is_null = false;
This variable is assigned on L740, so no need to separate declaration and definition.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/sorter.cc@742
PS8, Line 742: ValueType
Could use the actual type, i.e. StringValue here for readability.
http://gerrit.cloudera.org:8080/#/c/20496/8/be/src/runtime/sorter.cc@757
PS8, Line 757: auto
Writing out the actual type (SlotDescriptor*) would improve readability.
http://gerrit.cloudera.org:8080/#/c/20496/8/testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test
File testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test:
http://gerrit.cloudera.org:8080/#/c/20496/8/testdata/workloads/functional-query/queries/QueryTest/analytic-fns-tpcds-partitioned-topn.test@299
PS8, Line 299: # Same as above, but calculate aggregates over all the result columns to ensure that the right
Could include in the comment that in this test
memory pressure does force in-memory partitions to be evicted.
--
To view, visit http://gerrit.cloudera.org:8080/20496
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I741c3a5f12ab620b6b64b57d4c89b5f8e056efd3
Gerrit-Change-Number: 20496
Gerrit-PatchSet: 8
Gerrit-Owner: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2023 13:08:54 +0000
Gerrit-HasComments: Yes