[Impala-ASF-CR] IMPALA-4192: Move static state from DataSink into a DataSinkConfig

["Csaba Ringhofer (Code Review)" <ge...@cloudera.org>]

Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/14941 )

Change subject: IMPALA-4192: Move static state from DataSink into a DataSinkConfig
......................................................................


Patch Set 1: Code-Review+1

(5 comments)

http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h
File be/src/exec/data-sink.h:

http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.h@67
PS1, Line 67: tsink_
Can you add that this is owned by FragmentInstanceState?


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@55
PS1, Line 55: thrift_sink
This is already stored in tsink_.


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@56
PS1, Line 56:   const vector<TExpr>& thrift_output_exprs = thrift_sink.output_exprs;
This is not used.


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/data-sink.cc@60
PS1, Line 60: thrift_sink.type
I would prefer to create DataSinkConfig sub-classes for HBase + Kudu + other sinks in this patch and move this logic to their CreateSink() overrides.

The reason is that these classes will be created in the future anyway (if we want to move more static code out of the DataSinks), and this mixed switch/override solution just makes things harder to understand.


http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h
File be/src/exec/partitioned-hash-join-builder.h:

http://gerrit.cloudera.org:8080/#/c/14941/1/be/src/exec/partitioned-hash-join-builder.h@45
PS1, Line 45: Since it is expected to only be created
            : /// and used by PartitionedHashJoinPlanNode only, the DataSinkConfig::Init() and
            : /// DataSinkConfig::CreateSink() are not implemented for it.
This inheritance doesn't seem too intuitive, we only reuse input_row_desc_ from DataSinkConfig if I saw correctly. Is this a temporary thing related to IMPALA-4224? If not, then I would prefer to add input_row_desc_ here and remove the inheritance.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d5b4226f6cec5305b0ec9a25c5f18b5521c8dd2
Gerrit-Change-Number: 14941
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Jan 2020 15:25:50 +0000
Gerrit-HasComments: Yes