majetideepak commented on code in PR #8470:
URL: https://github.com/apache/incubator-gluten/pull/8470#discussion_r1910471098
##########
cpp/velox/compute/WholeStageResultIterator.cc:
##########
@@ -192,10 +192,13 @@ std::shared_ptr<velox::core::QueryCtx>
WholeStageResultIterator::createNewVeloxQ
std::shared_ptr<ColumnarBatch> WholeStageResultIterator::next() {
tryAddSplitsToTask();
+ if (task_ == nullptr) {
+ return nullptr;
+ }
if (task_->isFinished()) {
return nullptr;
}
- velox::RowVectorPtr vector;
+ velox::RowVectorPtr vector = nullptr;
Review Comment:
redundant since `RowVectorPtr` is a shared pointer.
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -312,7 +312,7 @@ std::shared_ptr<const core::ConstantTypedExpr>
SubstraitVeloxExprConverter::lite
std::vector<variant> variants;
variants.reserve(literals.size());
VELOX_CHECK_GE(literals.size(), 0, "List should have at least one item.");
- std::optional<TypePtr> literalType;
+ std::optional<TypePtr> literalType = std::nullopt;
Review Comment:
Not required. The constructor of std::optional does this.
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -265,20 +272,25 @@ MemoryUsageStats collectVeloxMemoryUsageStats(const
velox::memory::MemoryPool* p
}
MemoryUsageStats collectGlutenAllocatorMemoryUsageStats(const MemoryAllocator*
allocator) {
- MemoryUsageStats stats;
+ MemoryUsageStats stats{};
Review Comment:
This is redundant.
##########
cpp/velox/memory/VeloxMemoryManager.cc:
##########
@@ -215,7 +215,11 @@ class ArbitratorFactoryRegister {
};
VeloxMemoryManager::VeloxMemoryManager(const std::string& kind,
std::unique_ptr<AllocationListener> listener)
- : MemoryManager(kind), listener_(std::move(listener)) {
+ : MemoryManager(kind) {
+ if (listener == nullptr) {
+ throw gluten::GlutenException("VeloxMemoryManager failed");
+ }
+ listener_(std::move(listener))
Review Comment:
Why move this here?
You can keep it as is and check for `listener_` above
##########
cpp/velox/substrait/SubstraitToVeloxExpr.cc:
##########
@@ -433,7 +433,7 @@ VectorPtr SubstraitVeloxExprConverter::literalsToVector(
// Handle EmptyList and List together since the children could be either
case.
case ::substrait::Expression_Literal::LiteralTypeCase::kEmptyList:
case ::substrait::Expression_Literal::LiteralTypeCase::kList: {
- ArrayVectorPtr elements;
+ ArrayVectorPtr elements = nullptr;
Review Comment:
false positive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
