You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Michael Smith (Code Review)" <ge...@cloudera.org> on 2023/09/19 21:54:59 UTC
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Michael Smith has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20494
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
117 files changed, 381 insertions(+), 363 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/1
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#12).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-init.cc
M be/src/service/workload-management.cc
M be/src/service/workload-management.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
132 files changed, 430 insertions(+), 407 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/12
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc@70
PS2, Line 70: // Passing runnable in to this method (rather than reading from this->runnable())
> Based on the Notes at https://en.cppreference.com/w/cpp/utility/functional/
I've tried it with a small test file and I think you're right, std::bind seems to keep a copy of the shared_ptr and increment its counter.
But it's not easy to see, so I think we should explain it in a comment at least.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc@720
PS2, Line 720: sub_ranges
> sub_ranges is called in a loop, we need to make a copy each call.
Right, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 15:27:33 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc
File be/src/benchmarks/parquet-delta-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc@236
PS16, Line 236: move
> The compiler doesn't have a problem with it, but I can go ahead and be spec
We explicitly specify 'using std::move' in common/names.h. I'd rather stick with the unornamented one in .cc files.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h
File be/src/codegen/codegen-anyval-read-write-info.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h@173
PS16, Line 173: // Creates a PHI node that will have the value 'non_null_value' if the incoming block is
: // 'non_null_block_' and the value 'null_value' if it is 'null_block_'.
: llvm::PHINode* CodegenNullPhiNode(
: llvm::Value* non_null_value, llvm::Value* null_value, const std::string& name = "");
:
: // Creates a PHI node the value of which tells whether it was reached from the non-null
: // or the null path, i.e. whether this CodegenAnyValReadWriteInfo is null.
: llvm::PHINode* CodegenIsNullPhiNode(const std::string& name = "");
> 'name' can be made mandatory here as well.
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc@697
PS16, Line 697: llvm::PHINode* LlvmCodeGen::CreateBinaryPhiNode(LlvmBuilder* builder, llvm::Value* value1,
: llvm::Value* value2, llvm::BasicBlock* incoming_block1,
: llvm::BasicBlock* incoming_block2, const string& name) {
> This patch is pretty huge, and focused on the performance checks. I'll crea
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc@86
PS16, Line 86: move
> It includes common/names.h. Maybe it's the _GLIBCXX_UTILITY guard that's ca
Covered by common/names.h.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38
PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator)
> This patch is pretty huge, and focused on the performance checks. I'll crea
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h
File be/src/transport/TSaslServerTransport.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@112
PS16, Line 112:
: /**
: * Construct a new TSaslTrasnport, passing in the components of the definition.
: */
: TSaslServerTransport(std::string mechanism,
: std::string protocol,
: std::string serverName,
: std::string realm,
: unsigned flags,
: std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks,
: std::shared_ptr<TTransport> transport);
> This patch is pretty huge, and focused on the performance checks. I'll crea
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@152
PS16, Line 152: /**
: * Create a new Factor for a server definition.
: * Provides a single definition for the server, others may be added.
: */
: Factory(const std::string& mechanism, const std::string& protocol,
: const std::string& serverName, const std::string& realm,
: unsigned flags, std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks)
: : TTransportFactory() {
: addServerDefinition(mechanism, protocol, serverName, realm, flags,
: move(props), move(callbacks));
: }
> This patch is pretty huge, and focused on the performance checks. I'll crea
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h
File be/src/util/debug-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h@69
PS16, Line 69: const std::string& separator = ","
> All callers pass '\n' as separator. Perhaps this can be replaced with Print
https://issues.apache.org/jira/browse/IMPALA-13502
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc
File be/src/util/parquet-reader.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc@62
PS16, Line 62: return tproto_factory.getProtocol(move(mem));
: } else {
: TBinaryProtocolFactoryT<TMemoryBuffer> tproto_factory;
: return tproto_factory.getProtocol(move(mem));
> clion says this should be std::move.
Covered by common/names.h.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc@65
PS16, Line 65: move
> clion says this should be std::move.
Updated this just to be consistent with the others below. SampleFunction is a boost::function, so it also implicitly matches boost::move and we need to be explicit.
We could definitely also do a pass and convert lots of boost classes to std classes, like boost::function -> std::function. That's for another ticket.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:03:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#6).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
120 files changed, 425 insertions(+), 399 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/6
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#3).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
118 files changed, 395 insertions(+), 372 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/3
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14313/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2023 23:49:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#15).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-init.cc
M be/src/service/workload-management.cc
M be/src/service/workload-management.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
132 files changed, 518 insertions(+), 502 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/15
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 15
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14: Code-Review+1
(1 comment)
This might be a good opportunity to run clang-format-diff against HEAD^ and take any small style fixes.
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG@12
PS14, Line 12: Updates based on how values are used:
> "Safe to always follow new code": each individual change is pretty small an
Ack
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 17:37:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 19: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:18:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 19: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 23:42:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 2:
Negligible (possibly positive) impact on performance
+----------+-----------------------+---------+------------+------------+----------------+
| Workload | File Format | Avg (s) | Delta(Avg) | GeoMean(s) | Delta(GeoMean) |
+----------+-----------------------+---------+------------+------------+----------------+
| TPCH(42) | parquet / none / none | 3.56 | -0.16% | 2.35 | -0.27% |
+----------+-----------------------+---------+------------+------------+----------------+
+----------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| Query | Avg(s) | Base Avg(s) | Delta(Avg) | StdDev(%) | Base StdDev(%) | Iters | Median Diff(%) | MW Zval | Tval |
+----------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
| TPCH-Q1 | 4.74 | 4.64 | +2.08% | 1.58% | 1.40% | 50 | +2.12% | 6.44 | 6.88 |
| TPCH-Q13 | 5.96 | 5.89 | +1.17% | 0.57% | 1.08% | 50 | +1.63% | 6.30 | 6.75 |
| TPCH-Q6 | 0.69 | 0.69 | +1.11% | 3.58% | 3.59% | 50 | +0.23% | 1.72 | 1.54 |
| TPCH-Q20 | 1.56 | 1.55 | +0.80% | 2.30% | 2.12% | 50 | +0.06% | 1.10 | 1.80 |
| TPCH-Q14 | 1.65 | 1.64 | +0.30% | 2.37% | 1.89% | 50 | +0.07% | 1.60 | 0.71 |
| TPCH-Q15 | 2.28 | 2.28 | +0.10% | 2.05% | 1.48% | 50 | +0.02% | 0.17 | 0.28 |
| TPCH-Q21 | 17.83 | 17.81 | +0.08% | 0.57% | 0.43% | 50 | +0.01% | 0.55 | 0.79 |
| TPCH-Q8 | 2.49 | 2.49 | +0.04% | 2.18% | 2.08% | 50 | -0.00% | -0.00 | 0.09 |
| TPCH-Q9 | 9.75 | 9.77 | -0.14% | 0.82% | 0.87% | 50 | -0.09% | -1.32 | -0.82 |
| TPCH-Q19 | 1.24 | 1.24 | -0.23% | 1.24% | 1.50% | 50 | -0.03% | -0.79 | -0.82 |
| TPCH-Q3 | 3.46 | 3.47 | -0.35% | 1.38% | 1.20% | 50 | -0.03% | -1.27 | -1.35 |
| TPCH-Q11 | 0.54 | 0.55 | -0.33% | 4.47% | 4.38% | 50 | -0.16% | -1.32 | -0.38 |
| TPCH-Q5 | 1.95 | 1.96 | -0.58% | 1.44% | 1.69% | 50 | -0.06% | -1.47 | -1.85 |
| TPCH-Q12 | 1.48 | 1.49 | -0.67% | 2.11% | 2.14% | 50 | -0.12% | -1.49 | -1.59 |
| TPCH-Q4 | 1.89 | 1.91 | -0.81% | 1.57% | 1.72% | 50 | -0.05% | -2.35 | -2.49 |
| TPCH-Q2 | 0.66 | 0.67 | -0.76% | 3.89% | 4.17% | 50 | -0.16% | -1.02 | -0.94 |
| TPCH-Q16 | 1.37 | 1.39 | -1.03% | 2.09% | 1.18% | 50 | -0.04% | -2.37 | -3.05 |
| TPCH-Q17 | 1.86 | 1.89 | -1.47% | 4.69% | 6.37% | 50 | -0.27% | -0.96 | -1.32 |
| TPCH-Q22 | 1.75 | 1.79 | -1.96% | 1.72% | 4.41% | 50 | -0.19% | -3.61 | -2.94 |
| TPCH-Q10 | 3.03 | 3.07 | -1.04% | 1.85% | 1.94% | 50 | -1.42% | -2.21 | -2.76 |
| TPCH-Q7 | 2.82 | 2.85 | -1.07% | 1.78% | 2.04% | 50 | -1.54% | -2.53 | -2.81 |
| TPCH-Q18 | 9.36 | 9.48 | -1.29% | 1.28% | 1.17% | 50 | -1.51% | -5.01 | -5.30 |
+----------+--------+-------------+------------+-----------+----------------+-------+----------------+---------+-------+
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Sep 2023 17:33:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14060/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 19:40:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 2:
(8 comments)
I've only gone through the first 45 files, will try to finish it later.
Also, running an ASAN test job would be a good idea because this patch touches ownership in many places.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc
File be/src/codegen/codegen-anyval-read-write-info.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc@145
PS2, Line 145: name
Couldn't we take it by const reference? Now LlvmCodeGen::CreateBinaryPhiNode() takes a const ref.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc@150
PS2, Line 150: name
Couldn't we take it by const ref if we can do it for CodegenNullPhiNode()?
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/TAcceptQueueServer.cpp@210
PS2, Line 210: const shared_ptr<TTransport>& io_transport, const shared_ptr<TTransport>& client) {
Is there a reason why we don't take 'io_transport' and 'client' as raw pointers?
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc@627
PS2, Line 627: string&
Is it OK if we modify this string? GetUsernameFromBasicAuthHeader() calls TryStripPrefixString() from gutil, which swaps it with its argument 'result' (https://github.com/apache/impala/blob/4d15558b5eaa69e872917c8bbf69dc1dc2146bc5/be/src/gutil/strings/strip.cc#L35)
In GetUsernameFromBasicAuthHeader() we pass an empty string as the 'result' argument, so 'auth_header' may end up being empty.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc@641
PS2, Line 641: string&
See L627.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc@70
PS2, Line 70: // Passing runnable in to this method (rather than reading from this->runnable())
This comment indicates that taking by (const) reference may not be correct here.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc@720
PS2, Line 720: sub_ranges
InitRange() takes 'sub_ranges' by value and moves it into ScanRange::Reset(). Shouldn't SingleReaderTestBody() also take 'sub_ranges' by value and move it into InitRange()?
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc@1085
PS2, Line 1085: sub_ranges
See L720, although here we also use 'sub_ranges' on L1097. I'm not sure whether it's better to leave it like this or take 'sub_ranges' by value and move it only on L1097. The second option is more error-prone in case we add a third usage later, although this is true in every function where we use the pattern "accept by value - move into place".
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2023 16:09:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Nov 2023 23:36:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has removed a vote on this change.
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
> Patch Set 14: Code-Review+1
>
> (1 comment)
>
> This might be a good opportunity to run clang-format-diff against HEAD^ and take any small style fixes.
Ran clang-format-diff and picked up most of the fixes. I left out bigger changes in internal-server-test.cc (we’re pretty inconsistent about how much we indent 'public:' and 'private:') and be/src/transport (which are copied from Thrift and use pretty different style).
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 20:57:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 17:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17329/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 17
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:31:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 2:
> Patch Set 1:
>
> Started a perf run at https://jenkins.impala.io/job/gerrit-verify-dryrun-external/2169/
Oops, meant https://jenkins.impala.io/job/perf-AB-test/495/
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 20 Sep 2023 00:03:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 4:
(10 comments)
I went through the rest.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h@38
PS4, Line 38: allocator
Shouldn't we take it by raw pointer or reference?
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1502
PS4, Line 1502: session
Could take by raw pointer, as in for example SetupResultsCacheing().
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1510
PS4, Line 1510: session
Could take by raw pointer, as in for example SetupResultsCacheing().
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@889
PS4, Line 889: query_record
Couldn't this be a const ref?
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@1464
PS4, Line 1464: &
We take 'session_state' in ImpalaServer::RegisterQuery() by pointer, shouldn't we do the same here? Usually when the value is not const we take it by pointer.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@106
PS4, Line 106: std::string&, std::string&
Can't these be const refs?
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@111
PS4, Line 111: std::string&
Can't this be a const ref?
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59
PS4, Line 59: flags
'flags' is an unsigned int, no need to move it. Or would you like to move it for consistency?
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59
PS4, Line 59: move(props), move(callbacks)
'props' and 'callbacks' are taken by reference.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@55
PS4, Line 55: int256_t
I wonder if we ever have unaligned 'int256_t's. If we do, this might cause a problem if the compiler generates alignment-sensitive instructions here. I don't know if that's possible... I came across a similar thing in https://gerrit.cloudera.org/#/c/16134/, but it's possible that that commit also ensures this doesn't happen here.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 14:32:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/5/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/5/be/src/rpc/thrift-thread.cc@72
PS5, Line 72: promise->Set(get_current());
> We could still say something about why we need 'runnable' as an extra argum
Done
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Sep 2023 17:25:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#9).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
120 files changed, 428 insertions(+), 404 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/9
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#8).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
120 files changed, 429 insertions(+), 404 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/8
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14254/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 8
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Oct 2023 21:46:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc
File be/src/benchmarks/parquet-delta-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc@236
PS16, Line 236: move
> clion says this should be std::move, here and below.
The compiler doesn't have a problem with it, but I can go ahead and be specific.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc@86
PS16, Line 86: move
> clion says this should be std::move. Maybe because there is no "using names
It includes common/names.h. Maybe it's the _GLIBCXX_UTILITY guard that's causing problems.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 17:34:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@14
PS10, Line 14: move into place
> question: I thought std::move is implied by compiler in some obvious case.
clang-tidy would complain. Most cases of automatic "move" are https://en.cppreference.com/w/cpp/language/copy_elision.
As far as I know, there are no automatic moves in a constructor; the compiler could probably figure them out, but it has to ensure there are no other references within the constructor (including any function calls) and I don't think any do that analysis.
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@19
PS10, Line 19: cascading changes to other calls.
> Curious, which call path has the deepest cascading change?
I'll have to rediscover that. I'm going to work on splitting this up into a couple patches to make the types of changes clearer.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 23:39:31 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#5).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/benchmarks/row-batch-serialize-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/row-batch-serialize-test.cc
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
120 files changed, 421 insertions(+), 399 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/5
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 07 Nov 2023 13:00:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14085/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Sep 2023 17:56:18 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#11).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-init.cc
M be/src/service/workload-management.cc
M be/src/service/workload-management.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
132 files changed, 431 insertions(+), 408 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/11
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/11/be/src/benchmarks/lock-benchmark.cc
File be/src/benchmarks/lock-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20494/11/be/src/benchmarks/lock-benchmark.cc@154
PS11, Line 154: new thread(consume_fn, num_per_consumer, &data->value));
> This isn't right, this would do multiple moves on the same source.
Done
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 21:45:38 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 13: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11077/
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 13
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 03:12:50 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#18).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/workload-management-init.cc
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-worker.cc
M be/src/service/workload-management-worker.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
M be/src/workload_mgmt/workload-management.cc
M be/src/workload_mgmt/workload-management.h
134 files changed, 508 insertions(+), 491 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/18
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp
File be/src/transport/TSaslTransport.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp@43
PS13, Line 43: memBuf_(new TMemoryBuffer(DEFAULT_MEM_BUF_SIZE, transport_->getConfiguration())),
> Need to update to transport_
Done
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp@53
PS13, Line 53: memBuf_(new TMemoryBuffer(transport_->getConfiguration())),
> Same
Done
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:14:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11079/
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 21:38:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
Custom cluster tests failed on https://issues.apache.org/jira/browse/IMPALA-13498, otherwise run was successful.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 23:45:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 1:
Started a perf run at https://jenkins.impala.io/job/gerrit-verify-dryrun-external/2169/
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 21:57:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 19:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11091/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 19
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:18:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16: Code-Review+1
(1 comment)
One small comment, otherwise +1.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38
PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator)
Can't it be a raw pointer? And then KrpcDataStreamSender::Channel::Init() in krpc-data-stream-sender.cc could also take a raw pointer.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 16:07:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38
PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator)
> Can't it be a raw pointer? And then KrpcDataStreamSender::Channel::Init() i
It could, but I decided to reduce the number of incidental changes to make this patch easier to review.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 16:09:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp
File be/src/transport/TSaslTransport.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp@43
PS13, Line 43: memBuf_(new TMemoryBuffer(DEFAULT_MEM_BUF_SIZE, transport->getConfiguration())),
Need to update to transport_
http://gerrit.cloudera.org:8080/#/c/20494/13/be/src/transport/TSaslTransport.cpp@53
PS13, Line 53: memBuf_(new TMemoryBuffer(transport->getConfiguration())),
Same
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 13
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:01:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#14).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-init.cc
M be/src/service/workload-management.cc
M be/src/service/workload-management.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
132 files changed, 432 insertions(+), 409 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/14
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 15:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17310/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 15
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 21:21:39 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG@12
PS14, Line 12: Updates based on how values are used:
> Regarding potential conflict resolution if backporting is needed after this
"Safe to always follow new code": each individual change is pretty small and was forced by the new clang-tidy check. I think carrying any of these changes individually as part of a backport would be fine as long as they compile and pass tests.
I previously ran perf-AB-test and found not much (see comment from Sept 20, 2023). I don't think that's surprising, most of the changes are off critical paths, and the ones that are generally were copying small structs (on the order of 10s of bytes).
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 17:20:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14029/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 22:23:55 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14051/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2023 18:47:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@49
PS4, Line 49: static T SafeMultiply(T a, T b, bool may_overflow) {
How much does it matter that the SafeMultiply here and the int256_t version have different signatures? (I honestly don't know the answer.)
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 15:51:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10:
(3 comments)
I just have 1 nit and few questions.
This change is also pretty big and spread around. What is the recommendation if we need to backport to older branch and found a conflict due to absence/presence of this commit? What if the conflict is in the middle of long call path where the whole path is modified by this change?
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@9
PS10, Line 9: performance-unnecessary-value-param
nit: put link in commit message explaining what this is about.
https://clang.llvm.org/extra/clang-tidy/checks/performance/unnecessary-value-param.html
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@14
PS10, Line 14: move into place
question: I thought std::move is implied by compiler in some obvious case. Does clang-tidy complain about missing std::move?
https://stackoverflow.com/questions/9779079/why-does-c11-have-implicit-moves-for-value-parameters-but-not-for-rvalue-para
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@19
PS10, Line 19: cascading changes to other calls.
Curious, which call path has the deepest cascading change?
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 21:14:57 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11079/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:15:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17280/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 21:54:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/11/be/src/benchmarks/lock-benchmark.cc
File be/src/benchmarks/lock-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20494/11/be/src/benchmarks/lock-benchmark.cc@154
PS11, Line 154: new thread(move(consume_fn), num_per_consumer, &data->value));
This isn't right, this would do multiple moves on the same source.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 21:30:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 13:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11077/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 13
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 21:46:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h
File be/src/util/debug-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h@69
PS16, Line 69: const std::string& separator = ","
> https://issues.apache.org/jira/browse/IMPALA-13502
I think I'm going to skip this one. No real reason to change it.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:14:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(10 comments)
In several placed, clion gave this warning over move syntax:
Unqualified call to 'std::move'
Maybe all new 'move' in this patch should be addressed as 'std::move' to be safe.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc
File be/src/benchmarks/parquet-delta-benchmark.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/benchmarks/parquet-delta-benchmark.cc@236
PS16, Line 236: move
clion says this should be std::move, here and below.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h
File be/src/codegen/codegen-anyval-read-write-info.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/codegen-anyval-read-write-info.h@173
PS16, Line 173: // Creates a PHI node that will have the value 'non_null_value' if the incoming block is
: // 'non_null_block_' and the value 'null_value' if it is 'null_block_'.
: llvm::PHINode* CodegenNullPhiNode(
: llvm::Value* non_null_value, llvm::Value* null_value, const std::string& name = "");
:
: // Creates a PHI node the value of which tells whether it was reached from the non-null
: // or the null path, i.e. whether this CodegenAnyValReadWriteInfo is null.
: llvm::PHINode* CodegenIsNullPhiNode(const std::string& name = "");
'name' can be made mandatory here as well.
"val_ptr_phi" in filter-context.cc can be a constant string.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@629
PS16, Line 629: string
> Right. I mention this change in the commit message.
Ack
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@664
PS16, Line 664: const string& auth_header
> It's used at line 670.
Ack
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/thrift-thread.cc@86
PS16, Line 86: move
clion says this should be std::move. Maybe because there is no "using namespace std;" here?
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h
File be/src/transport/TSaslServerTransport.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@112
PS16, Line 112:
: /**
: * Construct a new TSaslTrasnport, passing in the components of the definition.
: */
: TSaslServerTransport(std::string mechanism,
: std::string protocol,
: std::string serverName,
: std::string realm,
: unsigned flags,
: std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks,
: std::shared_ptr<TTransport> transport);
I think this constructor is unused now. Please double check.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@152
PS16, Line 152: /**
: * Create a new Factor for a server definition.
: * Provides a single definition for the server, others may be added.
: */
: Factory(const std::string& mechanism, const std::string& protocol,
: const std::string& serverName, const std::string& realm,
: unsigned flags, std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks)
: : TTransportFactory() {
: addServerDefinition(mechanism, protocol, serverName, realm, flags,
: move(props), move(callbacks));
: }
I think this constructor is unused now. Please double check.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h
File be/src/util/debug-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/debug-util.h@69
PS16, Line 69: const std::string& separator = ","
All callers pass '\n' as separator. Perhaps this can be replaced with PrintIdsInMultiLine() without separator parameter.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc
File be/src/util/parquet-reader.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/parquet-reader.cc@62
PS16, Line 62: return tproto_factory.getProtocol(move(mem));
: } else {
: TBinaryProtocolFactoryT<TMemoryBuffer> tproto_factory;
: return tproto_factory.getProtocol(move(mem));
clion says this should be std::move.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc
File be/src/util/periodic-counter-updater.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/util/periodic-counter-updater.cc@65
PS16, Line 65: move
clion says this should be std::move.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 02:33:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 11:
I've simplified this patch a bit. It's basically just address each warning from performance-unnecessary-value-param with either: move values into place, or use a const&.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 11
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 21:26:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 3:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc
File be/src/codegen/codegen-anyval-read-write-info.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc@145
PS2, Line 145: trin
> Couldn't we take it by const reference? Now LlvmCodeGen::CreateBinaryPhiNod
Oh yup, I think I looked at this file before CreateBinaryPhiNode. Makes sense for these to be const&.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/codegen/codegen-anyval-read-write-info.cc@150
PS2, Line 150: trin
> Couldn't we take it by const ref if we can do it for CodegenNullPhiNode()?
Done
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/TAcceptQueueServer.cpp
File be/src/rpc/TAcceptQueueServer.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/TAcceptQueueServer.cpp@210
PS2, Line 210: const shared_ptr<TTransport>& io_transport, const shared_ptr<TTransport>& client) {
> Is there a reason why we don't take 'io_transport' and 'client' as raw poin
Private helper method that's simpler to call with shared_ptr. Not really a consistent reason.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc@627
PS2, Line 627: string
> Is it OK if we modify this string? GetUsernameFromBasicAuthHeader() calls T
You're right, better to be safe here. I'll have to figure out another way to appease clang-tidy.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/authentication.cc@641
PS2, Line 641:
> See L627.
Done
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc@70
PS2, Line 70: // Passing runnable in to this method (rather than reading from this->runnable())
> This comment indicates that taking by (const) reference may not be correct
Based on the Notes at https://en.cppreference.com/w/cpp/utility/functional/bind, I think the std::bind call above saves a copy of the shared_ptr, thereby reference counting it. So I think this comment is incorrect. However I'm comfortable adding a clang-tidy exception if that doesn't seem clear.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc
File be/src/runtime/io/disk-io-mgr-test.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc@720
PS2, Line 720: sub_ranges
> InitRange() takes 'sub_ranges' by value and moves it into ScanRange::Reset(
sub_ranges is called in a loop, we need to make a copy each call.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/runtime/io/disk-io-mgr-test.cc@1085
PS2, Line 1085: sub_ranges
> See L720, although here we also use 'sub_ranges' on L1097. I'm not sure whe
We make multiple copies of sub_ranges in this function: here, and in a loop at L1094.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 3
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2023 18:19:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14031/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 23:12:31 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17293/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Oct 2024 16:42:07 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, Riza Suminto, gaurav singh, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#17).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/workload-management-init.cc
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-worker.cc
M be/src/service/workload-management-worker.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
M be/src/workload_mgmt/workload-management.cc
M be/src/workload_mgmt/workload-management.h
134 files changed, 507 insertions(+), 490 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/17
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 17
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/14//COMMIT_MSG@12
PS14, Line 12: Updates based on how values are used:
Regarding potential conflict resolution if backporting is needed after this patch, it looks like we are safe to always follow new code from this patch, is that right?
I'm also curious if there will be performance improvement from this change. Can you please run some microbenchmark like expr-benchmark or https://jenkins.impala.io/job/perf-AB-test/ before and after this patch?
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 00:17:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 14:
> Patch Set 14: Code-Review+1
>
> (1 comment)
>
> This might be a good opportunity to run clang-format-diff against HEAD^ and take any small style fixes.
That's going to add a lot more reformatting, but I'm open to it.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 14
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 18:00:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 18:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17330/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:37:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc@697
PS16, Line 697: llvm::PHINode* LlvmCodeGen::CreateBinaryPhiNode(LlvmBuilder* builder, llvm::Value* value1,
: llvm::Value* value2, llvm::BasicBlock* incoming_block1,
: llvm::BasicBlock* incoming_block2, const string& name) {
> nit: There are only few callsites calling this method. Maybe we can conside
Seems reasonable, I think there's only one that omits passing a name and it probably should specify one.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@629
PS16, Line 629: string
> This is not const string& because TryStripPrefixString mutate auth_header?
Right. I mention this change in the commit message.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@664
PS16, Line 664: const string& auth_header
> this is unused?
It's used at line 670.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 23:24:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place to efficiently handle literal values
- others take a const& of the value
GetUsernameFromBasicAuthHeader was updated to make a copy closer to
where it's needed, rather than relying on the caller setting it up.
Updates CodegenNullPhiNode to use its 'name' parameter. Only impacts two
calls in filter-context.cc.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Reviewed-on: http://gerrit.cloudera.org:8080/20494
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/catalog/workload-management-init.cc
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/avro/hdfs-avro-scanner-test.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/iceberg-buffered-delete-sink.cc
M be/src/exec/iceberg-buffered-delete-sink.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-filter.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tuple-cache-mgr-test.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/service/internal-server-test.cc
M be/src/service/query-state-record.cc
M be/src/service/query-state-record.h
M be/src/service/workload-management-worker.cc
M be/src/service/workload-management-worker.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/in-list-filter.h
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/network-util-test.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/summary-util-test.cc
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
M be/src/workload_mgmt/workload-management.cc
M be/src/workload_mgmt/workload-management.h
134 files changed, 508 insertions(+), 491 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 20
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38
PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator)
> It could, but I decided to reduce the number of incidental changes to make
I think seeing a 'const shared_ptr<>&' in general is suspicious, and we could do a pass to replace those at some point.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 16:10:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/20494/1/be/src/runtime/bufferpool/buffer-allocator.h
File be/src/runtime/bufferpool/buffer-allocator.h:
http://gerrit.cloudera.org:8080/#/c/20494/1/be/src/runtime/bufferpool/buffer-allocator.h@195
PS1, Line 195: int64_t SumOverArenas(const std::function<int64_t(FreeBufferArena* arena)>& compute_fn) const;
line too long (96 > 90)
http://gerrit.cloudera.org:8080/#/c/20494/1/be/src/scheduling/cluster-membership-mgr.cc
File be/src/scheduling/cluster-membership-mgr.cc:
http://gerrit.cloudera.org:8080/#/c/20494/1/be/src/scheduling/cluster-membership-mgr.cc@99
PS1, Line 99: const ClusterMembershipMgr::SnapshotPtr& snapshot) { this->UpdateMetrics(snapshot); });
line too long (93 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 19 Sep 2023 21:56:04 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello Daniel Becker, gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#4).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-anyval-read-write-info.h
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
118 files changed, 396 insertions(+), 373 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/4
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Hello gsingh@cloudera.com, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/20494
to look at the new patch set (#2).
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
IMPALA-12390 (part 4): Enable unnecessary-value-param
Enables the clang-tidy performance-unnecessary-value-param check and
fixes any issues found with run_clang_tidy.sh.
Updates based on how values are used:
- constructors and functions that take ownership of a value accept by
value and move into place
- others take a const& of the value
- const shared_ptr& should be rare, as it's more flexible to take a
pointer to the value and deref the shared_ptr when calling
Some updates required cascading changes to other calls. I attempted to
dig down through how a value was used to determine whether other calls
should be updated to take by value or reference.
Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
---
M .clang-tidy
M be/src/benchmarks/lock-benchmark.cc
M be/src/benchmarks/parquet-delta-benchmark.cc
M be/src/catalog/catalog-service-client-wrapper.h
M be/src/codegen/codegen-anyval-read-write-info.cc
M be/src/codegen/codegen-symbol-emitter.h
M be/src/codegen/llvm-codegen-cache-test.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/common/init.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/json/json-parser.h
M be/src/exec/parquet/hdfs-parquet-scanner.h
M be/src/exec/parquet/hdfs-parquet-table-writer.cc
M be/src/exec/parquet/parquet-column-chunk-reader.cc
M be/src/exec/parquet/parquet-common-test.cc
M be/src/exec/parquet/parquet-delta-coder-test.cc
M be/src/exec/parquet/parquet-page-reader.h
M be/src/exec/scanner-context.h
M be/src/exprs/expr-test.cc
M be/src/rpc/TAcceptQueueServer.cpp
M be/src/rpc/TAcceptQueueServer.h
M be/src/rpc/authentication.cc
M be/src/rpc/rpc-mgr-test.h
M be/src/rpc/thrift-server-test.cc
M be/src/rpc/thrift-thread.cc
M be/src/rpc/thrift-thread.h
M be/src/rpc/thrift-util.cc
M be/src/runtime/bufferpool/buffer-allocator.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/client-cache.cc
M be/src/runtime/client-cache.h
M be/src/runtime/exec-env.cc
M be/src/runtime/io/data-cache-trace-replayer.cc
M be/src/runtime/io/data-cache-trace-test.cc
M be/src/runtime/io/data-cache-trace.cc
M be/src/runtime/io/data-cache-trace.h
M be/src/runtime/io/data-cache.cc
M be/src/runtime/io/data-cache.h
M be/src/runtime/io/disk-io-mgr-test.cc
M be/src/runtime/io/disk-io-mgr.cc
M be/src/runtime/io/request-ranges.h
M be/src/runtime/krpc-data-stream-sender.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/multi-precision.h
M be/src/runtime/outbound-row-batch.h
M be/src/runtime/query-driver.cc
M be/src/runtime/query-driver.h
M be/src/runtime/reservation-manager.cc
M be/src/runtime/reservation-manager.h
M be/src/runtime/runtime-filter-bank.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/scheduling/admission-control-service.cc
M be/src/scheduling/admission-control-service.h
M be/src/scheduling/admission-controller-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/scheduling/admission-controller.h
M be/src/scheduling/admissiond-env.cc
M be/src/scheduling/cluster-membership-mgr.cc
M be/src/scheduling/cluster-membership-mgr.h
M be/src/scheduling/executor-group.cc
M be/src/scheduling/hash-ring-test.cc
M be/src/scheduling/scheduler-test-util.cc
M be/src/scheduling/scheduler-test-util.h
M be/src/service/client-request-state.cc
M be/src/service/impala-beeswax-server.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M be/src/statestore/statestore-service-client-wrapper.h
M be/src/statestore/statestore-subscriber-client-wrapper.h
M be/src/statestore/statestore-subscriber.cc
M be/src/statestore/statestore-subscriber.h
M be/src/statestore/statestore.h
M be/src/testutil/impalad-query-executor.h
M be/src/transport/THttpServer.cpp
M be/src/transport/THttpServer.h
M be/src/transport/THttpTransport.cpp
M be/src/transport/TSaslClientTransport.cpp
M be/src/transport/TSaslServerTransport.cpp
M be/src/transport/TSaslServerTransport.h
M be/src/transport/TSaslTransport.cpp
M be/src/util/bit-packing-test.cc
M be/src/util/bloom-filter-test.cc
M be/src/util/cache/lirs-cache.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
M be/src/util/decimal-util.h
M be/src/util/disk-info.cc
M be/src/util/disk-info.h
M be/src/util/in-list-filter-test.cc
M be/src/util/in-list-filter.cc
M be/src/util/internal-queue.h
M be/src/util/jwt-util-internal.h
M be/src/util/jwt-util.cc
M be/src/util/kudu-status-util.h
M be/src/util/logging-support-test.cc
M be/src/util/mpfit-util.cc
M be/src/util/os-util-test.cc
M be/src/util/parquet-reader.cc
M be/src/util/periodic-counter-updater.cc
M be/src/util/runtime-profile-counters.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/simple-logger-test.cc
M be/src/util/simple-logger.cc
M be/src/util/simple-logger.h
M be/src/util/symbols-util-test.cc
M be/src/util/thread.cc
M be/src/util/thread.h
M be/src/util/tuple-row-compare-test.cc
117 files changed, 383 insertions(+), 364 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/94/20494/2
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 2
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/14052/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Thu, 21 Sep 2023 20:15:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 4:
(12 comments)
> Also, running an ASAN test job would be a good idea because this patch touches ownership in many places.
I ran an ASAN job and it passed. I'll want to do another one after addressing all review comments.
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/2/be/src/rpc/thrift-thread.cc@70
PS2, Line 70: // Passing runnable in to this method (rather than reading from this->runnable())
> I've tried it with a small test file and I think you're right, std::bind se
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/runtime/outbound-row-batch.h@38
PS4, Line 38: allocator
> Shouldn't we take it by raw pointer or reference?
Yeah. I started using that pattern partway through, so missed some.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h
File be/src/service/impala-server.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1502
PS4, Line 1502: session
> Could take by raw pointer, as in for example SetupResultsCacheing().
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.h@1510
PS4, Line 1510: session
> Could take by raw pointer, as in for example SetupResultsCacheing().
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@889
PS4, Line 889: query_record
> Couldn't this be a const ref?
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/service/impala-server.cc@1464
PS4, Line 1464: &
> We take 'session_state' in ImpalaServer::RegisterQuery() by pointer, should
Right, that's a convention I still don't quite get but will follow.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@106
PS4, Line 106: std::string&, std::string&
> Can't these be const refs?
Yeah. Implementations got confusing, but const& should be fine for them too.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/THttpServer.h@111
PS4, Line 111: std::string&
> Can't this be a const ref?
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp
File be/src/transport/TSaslServerTransport.cpp:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59
PS4, Line 59: move(props), move(callbacks)
> 'props' and 'callbacks' are taken by reference.
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/transport/TSaslServerTransport.cpp@59
PS4, Line 59: flags
> 'flags' is an unsigned int, no need to move it. Or would you like to move i
Done
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@49
PS4, Line 49: static T SafeMultiply(T a, T b, bool may_overflow) {
> How much does it matter that the SafeMultiply here and the int256_t version
I think SafeMultiply below isn't actually a template specialization.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@55
PS4, Line 55: int256_t
> I wonder if we ever have unaligned 'int256_t's. If we do, this might cause
I'm not sure. This call is flagged because the operators for int256_t all take values by const&, so we're making an unnecessary copy.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 4
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Sep 2023 19:11:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20494/5/be/src/rpc/thrift-thread.cc
File be/src/rpc/thrift-thread.cc:
http://gerrit.cloudera.org:8080/#/c/20494/5/be/src/rpc/thrift-thread.cc@72
PS5, Line 72: promise->Set(get_current());
We could still say something about why we need 'runnable' as an extra argument instead of using 'this'. We could point to the comment on L35.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h
File be/src/util/decimal-util.h:
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@49
PS4, Line 49: static T SafeMultiply(T a, T b, bool may_overflow) {
> I think SafeMultiply below isn't actually a template specialization.
I think it's a regular overload right now. I don't think we can turn it into a template specialsation because of the difference in the argument type.
http://gerrit.cloudera.org:8080/#/c/20494/4/be/src/util/decimal-util.h@55
PS4, Line 55: int256_t
> I'm not sure. This call is flagged because the operators for int256_t all t
Ok, let's hope the change above solved it for all cases.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 5
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Sep 2023 17:20:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/9888/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Nov 2023 19:00:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 6: Code-Review+1
Thanks Michael.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 6
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <gs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Wed, 27 Sep 2023 09:44:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(3 comments)
I will continue reviewing this tonight. Looks like it is better to review using clion, in case IDE can point out something that vim can't.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc@697
PS16, Line 697: llvm::PHINode* LlvmCodeGen::CreateBinaryPhiNode(LlvmBuilder* builder, llvm::Value* value1,
: llvm::Value* value2, llvm::BasicBlock* incoming_block1,
: llvm::BasicBlock* incoming_block2, const string& name) {
nit: There are only few callsites calling this method. Maybe we can consider making 'name' mandatory instead of optional?
That way, the name.empty() branch is not needed anymore, and callsites might realize they can reuse a constant string.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@629
PS16, Line 629: string
This is not const string& because TryStripPrefixString mutate auth_header?
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/rpc/authentication.cc@664
PS16, Line 664: const string& auth_header
this is unused?
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Oct 2024 22:30:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 16:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc
File be/src/codegen/llvm-codegen.cc:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/codegen/llvm-codegen.cc@697
PS16, Line 697: llvm::PHINode* LlvmCodeGen::CreateBinaryPhiNode(LlvmBuilder* builder, llvm::Value* value1,
: llvm::Value* value2, llvm::BasicBlock* incoming_block1,
: llvm::BasicBlock* incoming_block2, const string& name) {
> Seems reasonable, I think there's only one that omits passing a name and it
This patch is pretty huge, and focused on the performance checks. I'll create a ticket to do some separate cleanup.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h
File be/src/runtime/outbound-row-batch.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/runtime/outbound-row-batch.h@38
PS16, Line 38: OutboundRowBatch(const std::shared_ptr<CharMemTrackerAllocator>& allocator)
> I think seeing a 'const shared_ptr<>&' in general is suspicious, and we cou
This patch is pretty huge, and focused on the performance checks. I'll create a ticket to do some separate cleanup.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h
File be/src/transport/TSaslServerTransport.h:
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@112
PS16, Line 112:
: /**
: * Construct a new TSaslTrasnport, passing in the components of the definition.
: */
: TSaslServerTransport(std::string mechanism,
: std::string protocol,
: std::string serverName,
: std::string realm,
: unsigned flags,
: std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks,
: std::shared_ptr<TTransport> transport);
> I think this constructor is unused now. Please double check.
This patch is pretty huge, and focused on the performance checks. I'll create a ticket to do some separate cleanup.
http://gerrit.cloudera.org:8080/#/c/20494/16/be/src/transport/TSaslServerTransport.h@152
PS16, Line 152: /**
: * Create a new Factor for a server definition.
: * Provides a single definition for the server, others may be added.
: */
: Factory(const std::string& mechanism, const std::string& protocol,
: const std::string& serverName, const std::string& realm,
: unsigned flags, std::map<std::string, std::string> props,
: std::vector<struct sasl_callback> callbacks)
: : TTransportFactory() {
: addServerDefinition(mechanism, protocol, serverName, realm, flags,
: move(props), move(callbacks));
: }
> I think this constructor is unused now. Please double check.
This patch is pretty huge, and focused on the performance checks. I'll create a ticket to do some separate cleanup.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 16
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 17:38:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 18: Code-Review+2
Thank you for addressing the comments and filing the follow up JIRAs.
Carry Daniel's vote.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 18
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Thu, 31 Oct 2024 18:16:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17281/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 12
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Mon, 28 Oct 2024 22:13:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-12390 (part 4): Enable unnecessary-value-param
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20494 )
Change subject: IMPALA-12390 (part 4): Enable unnecessary-value-param
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20494/10//COMMIT_MSG@14
PS10, Line 14: move into place
> clang-tidy would complain. Most cases of automatic "move" are https://en.cp
https://gerrit.cloudera.org/c/20494/10/be/src/exec/json/json-parser.h is an instance where clang-tidy definitely complained. I wouldn't have found that on my own.
--
To view, visit http://gerrit.cloudera.org:8080/20494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8aa5d98596d82f615a0a728e0235e7dd9d8b5003
Gerrit-Change-Number: 20494
Gerrit-PatchSet: 10
Gerrit-Owner: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Fri, 25 Oct 2024 23:41:49 +0000
Gerrit-HasComments: Yes