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 <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 06 Jan 2020 15:25:50 +0000 Gerrit-HasComments: Yes
