Fang-Yu Rao has posted comments on this change. ( http://gerrit.cloudera.org:8080/13882 )
Change subject: IMPALA-7984: Port UpdateFilter() and PublishFilter() to KRPC ...................................................................... Patch Set 11: (21 comments) I have addressed most of Thomas' previous comments. The current patch set passed the exhaustive private parameterized test as well. I will be working on incorporating sidecar now. Thanks. http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.h File be/src/runtime/runtime-filter-bank.h: http://gerrit.cloudera.org:8080/#/c/13882/3/be/src/runtime/runtime-filter-bank.h@97 PS3, Line 97: /// Makes a bloom_filter (aggregated globally from all producer fragments) available for > remove this Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/runtime/runtime-filter-bank.cc@224 PS2, Line 224: > line too long (93 > 90) Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.h File be/src/service/impala-internal-service.h: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.h@1 PS2, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > This whole file can be removed This file is kept in this patch set but will be removed in a follow up patch according to our discussion. http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.cc File be/src/service/impala-internal-service.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-internal-service.cc@1 PS2, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > This whole file can be removed This file is kept in this patch set but will be removed in a follow up patch according to our discussion. http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/service/impala-server.h@601 PS2, Line 601: /// the frontend. > Done Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter-test.cc File be/src/util/bloom-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter-test.cc@31 PS2, Line 31: #include "gen-cpp/data_stream_service.pb.h" > This can be moved up with the includes above it Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter.cc File be/src/util/bloom-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/bloom-filter.cc@208 PS2, Line 208: _directory()))[0])); > I don't think that you can use string::data() like this, see: http://www.cp Thanks for pointing this out! I have modified this expression according to your suggestion. http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter-test.cc File be/src/util/min-max-filter-test.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter-test.cc@28 PS2, Line 28: > This can be put with the block of includes right above it Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc File be/src/util/min-max-filter.cc: http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@378 PS2, Line 378: if (in_min_val < out_min_val) { > I don't remember if we talked about clang-format-diff yet, but if not you m Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@381 PS2, Line 381: TimestampValue in_max_val = TimestampValue::FromColumnValuePB(in.max()); > line too long (98 > 90) Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@404 PS2, Line 404: // Construct the Decimal min-max filter when the min-max filter information > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/13882/2/be/src/util/min-max-filter.cc@507 PS2, Line 507: if (Deci > I think this was unintentional? Thanks for pointing this out! I have removed it. http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/control_service.proto File common/protobuf/control_service.proto: http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/control_service.proto@274 PS2, Line 274: } > please omit this from the patch Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto File common/protobuf/data_stream_service.proto: http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@82 PS2, Line 82: // Log_2 of the bufferpool space required for this filter. > This can be removed (and the corresponding fields in the messages below). Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@89 PS2, Line 89: optional > Lets make everything optional Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@103 PS2, Line 103: > Put this in common.proto Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@127 PS2, Line 127: } > Remove this. Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@133 PS2, Line 133: > remove this, here and below Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@150 PS2, Line 150: // Latency for response in the receiving daemon in nanoseconds. > copy the comment from the equivalent field above Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/protobuf/data_stream_service.proto@196 PS2, Line 196: > copy over the comments that were with these in ImpalaInternalService.thrift Done http://gerrit.cloudera.org:8080/#/c/13882/2/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/13882/2/common/thrift/ImpalaInternalService.thrift@705 PS2, Line 705: service ImpalaInternalService { : : } > This can be removed This service is kept in this patch set but will be removed in a follow up patch according to our discussion. -- To view, visit http://gerrit.cloudera.org:8080/13882 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6b394796d250286510e157ae326882bfc01d387a Gerrit-Change-Number: 13882 Gerrit-PatchSet: 11 Gerrit-Owner: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Thu, 25 Jul 2019 15:22:11 +0000 Gerrit-HasComments: Yes
