[NSE-227]fix issues from codescan (#225)

* fix code scan Signed-off-by: Yuan Zhou <yuan.zhou@intel.com> * fix code scan * fix format Signed-off-by: Yuan Zhou <yuan.zhou@intel.com> * fix wrong patch Signed-off-by: Yuan Zhou <yuan.zhou@intel.com> * Klockwork issues fix * fix format Signed-off-by: Yuan Zhou <yuan.zhou@intel.com> * more fixes Signed-off-by: Yuan Zhou <yuan.zhou@intel.com> Co-authored-by: Rui Mo <rui.mo@intel.com> Co-authored-by: Hongze Zhang <hongze.zhang@intel.com>
oap-project · Apr 2, 2021 · 1441bcf · 1441bcf
1 parent c52cc1b
commit 1441bcf
Show file tree

Hide file tree

Showing 25 changed files with 189 additions and 101 deletions.
diff --git a/native-sql-engine/core/src/main/java/com/intel/oap/datasource/parquet/ParquetReader.java b/native-sql-engine/core/src/main/java/com/intel/oap/datasource/parquet/ParquetReader.java
@@ -119,6 +119,9 @@ public ArrowRecordBatch readNext() throws IOException {
     ArrowRecordBatchBuilderImpl recordBatchBuilderImpl =
         new ArrowRecordBatchBuilderImpl(recordBatchBuilder);
     ArrowRecordBatch batch = recordBatchBuilderImpl.build();
+    if (batch == null) {
+      throw new IllegalArgumentException("failed to build record batch");
+    }
     this.lastReadLength = batch.getLength();
     return batch;
   }

diff --git a/native-sql-engine/core/src/main/java/com/intel/oap/vectorized/JniUtils.java b/native-sql-engine/core/src/main/java/com/intel/oap/vectorized/JniUtils.java
@@ -183,11 +183,10 @@ private static File moveFileFromJarToTemp(String tmpDir, String libraryToLoad) t
     try (final InputStream is = JniUtils.class.getClassLoader().getResourceAsStream(libraryToLoad)) {
       if (is == null) {
         throw new FileNotFoundException(libraryToLoad);
-      } else {
-        try {
-          Files.copy(is, temp.toPath());
-        } catch (Exception e) {
-        }
+      }
+      try {
+        Files.copy(is, temp.toPath());
+      } catch (Exception e) {
       }
     }
     return temp;

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/code_generator.h b/native-sql-engine/cpp/src/codegen/arrow_compute/code_generator.h
@@ -67,7 +67,7 @@ class ArrowComputeCodeGenerator : public CodeGenerator {
 #endif
   }
 
-  ~ArrowComputeCodeGenerator() {
+  virtual ~ArrowComputeCodeGenerator() {
     expr_visitor_cache_.clear();
     visitor_list_.clear();
   }

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/basic_physical_kernels.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/basic_physical_kernels.cc
@@ -54,6 +54,7 @@ class ProjectKernel::Impl {
       auto field_node = std::dynamic_pointer_cast<gandiva::FieldNode>(node);
       input_field_list_.push_back(field_node->field());
     }
+    pool_ = nullptr;
   }
 
   arrow::Status MakeResultIterator(
@@ -125,6 +126,7 @@ ProjectKernel::ProjectKernel(arrow::compute::ExecContext* ctx,
                              const gandiva::NodeVector& project_list) {
   impl_.reset(new Impl(ctx, input_field_node_list, project_list));
   kernel_name_ = "ProjectKernel";
+  ctx_ = nullptr;
 }
 
 arrow::Status ProjectKernel::MakeResultIterator(
@@ -153,6 +155,7 @@ class FilterKernel::Impl {
       auto field_node = std::dynamic_pointer_cast<gandiva::FieldNode>(node);
       input_field_list_.push_back(field_node->field());
     }
+    pool_ = nullptr;
   }
 
   arrow::Status MakeResultIterator(
@@ -178,7 +181,8 @@ class FilterKernel::Impl {
                                                &condition_node_visitor));
     codegen_ctx->process_codes += condition_node_visitor->GetPrepare();
     for (auto header : condition_node_visitor->GetHeaders()) {
-      if (std::find(codegen_ctx->header_codes.begin(), codegen_ctx->header_codes.end(),
+      if (!header.empty() &&
+          std::find(codegen_ctx->header_codes.begin(), codegen_ctx->header_codes.end(),
                     header) == codegen_ctx->header_codes.end()) {
         codegen_ctx->header_codes.push_back(header);
       }

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/codegen_common.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/codegen_common.cc
@@ -332,7 +332,7 @@ gandiva::ExpressionPtr GetHash32Kernel(std::vector<gandiva::NodePtr> key_list,
     seed = func_node;
   }
   func_node_list.push_back(func_node);
-  return gandiva::TreeExprBuilder::MakeExpression(func_node_list[0],
+  return gandiva::TreeExprBuilder::MakeExpression(func_node_list.at(0),
                                                   arrow::field("hash_key", ret_type));
 }
 
@@ -605,12 +605,19 @@ arrow::Status CompileCodes(std::string codes, std::string signature) {
 std::string exec(const char* cmd) {
   std::array<char, 128> buffer;
   std::string result;
-  std::unique_ptr<FILE, decltype(&pclose)> pipe(popen(cmd, "r"), pclose);
+  FILE* file = popen(cmd, "r");
+  std::unique_ptr<FILE, decltype(&pclose)> pipe(file, pclose);
   if (!pipe) {
+    pclose(file);
     throw std::runtime_error("popen() failed!");
   }
-  while (fgets(buffer.data(), buffer.size(), pipe.get()) != nullptr) {
-    result += buffer.data();
+  try {
+    while (fgets(buffer.data(), sizeof buffer, pipe.get()) != nullptr) {
+      result += buffer.data();
+    }
+  } catch (...) {
+    pclose(file);
+    throw;
   }
   return result;
 }

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/codegen_node_visitor.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/codegen_node_visitor.cc
@@ -805,7 +805,8 @@ std::string CodeGenNodeVisitor::GetNaNCheckStr(std::string left, std::string rig
 std::string CodeGenNodeVisitor::CombineValidity(std::vector<std::string> validity_list) {
   bool first = true;
   std::stringstream out;
-  for (auto validity : validity_list) {
+  for (int i = 0; i < validity_list.size(); i++) {
+    auto validity = validity_list[i];
     if (first) {
       if (validity.compare("true") != 0) {
         out << validity;

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/conditioned_merge_join_kernel.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/conditioned_merge_join_kernel.cc
@@ -88,6 +88,8 @@ class ConditionedMergeJoinKernel::Impl {
       THROW_NOT_OK(GetIndexList(result_schema_, left_field_list_, right_field_list_, true,
                                 &exist_index_, &result_schema_index_list_));
     }
+
+    pool_ = nullptr;
   }
 
   arrow::Status MakeResultIterator(

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/conditioned_probe_kernel.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/conditioned_probe_kernel.cc
@@ -131,6 +131,8 @@ class ConditionedProbeKernel::Impl {
       THROW_NOT_OK(GetIndexList(result_schema_, left_field_list_, right_field_list_, true,
                                 &exist_index_, &result_schema_index_list_));
     }
+
+    pool_ = nullptr;
   }
 
   arrow::Status MakeResultIterator(
@@ -412,6 +414,9 @@ class ConditionedProbeKernel::Impl {
       auto iter = dependent_iter_list[0];
       auto typed_dependent =
           std::dynamic_pointer_cast<ResultIterator<HashRelation>>(iter);
+      if (typed_dependent == nullptr) {
+        throw std::runtime_error("casting on hash relation iterator failed");
+      }
       RETURN_NOT_OK(typed_dependent->Next(&hash_relation_));
 
       // chendi: previous result_schema_index_list design is little tricky, it
@@ -768,40 +773,44 @@ class ConditionedProbeKernel::Impl {
   case TypeTraits<InType>::type_id: {                                         \
     using ArrayType_ = precompile::TypeTraits<InType>::ArrayType;             \
     auto typed_first_key_arr = std::make_shared<ArrayType_>(key_payloads[0]); \
-    if (typed_first_key_arr->null_count() == 0) {                             \
-      fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {      \
-        return hash_relation_->Get(typed_key_array->GetView(i),               \
-                                   typed_first_key_arr->GetView(i));          \
-      };                                                                      \
-    } else {                                                                  \
-      fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {      \
-        if (typed_first_key_arr->IsNull(i)) {                                 \
-          return hash_relation_->GetNull();                                   \
-        } else {                                                              \
+    if (typed_first_key_arr) {                                                \
+      if (typed_first_key_arr->null_count() == 0) {                           \
+        fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {    \
           return hash_relation_->Get(typed_key_array->GetView(i),             \
                                      typed_first_key_arr->GetView(i));        \
-        }                                                                     \
-      };                                                                      \
+        };                                                                    \
+      } else {                                                                \
+        fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {    \
+          if (typed_first_key_arr->IsNull(i)) {                               \
+            return hash_relation_->GetNull();                                 \
+          } else {                                                            \
+            return hash_relation_->Get(typed_key_array->GetView(i),           \
+                                       typed_first_key_arr->GetView(i));      \
+          }                                                                   \
+        };                                                                    \
+      }                                                                       \
     }                                                                         \
   } break;
             PROCESS_SUPPORTED_TYPES(PROCESS)
 #undef PROCESS
             case TypeTraits<arrow::StringType>::type_id: {
               auto typed_first_key_arr = std::make_shared<StringArray>(key_payloads[0]);
-              if (typed_first_key_arr->null_count() == 0) {
-                fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {
-                  return hash_relation_->Get(typed_key_array->GetView(i),
-                                             typed_first_key_arr->GetString(i));
-                };
-              } else {
-                fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {
-                  if (typed_first_key_arr->IsNull(i)) {
-                    return hash_relation_->GetNull();
-                  } else {
+              if (typed_first_key_arr) {
+                if (typed_first_key_arr->null_count() == 0) {
+                  fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {
                     return hash_relation_->Get(typed_key_array->GetView(i),
                                                typed_first_key_arr->GetString(i));
-                  }
-                };
+                  };
+                } else {
+                  fast_probe = [this, typed_key_array, typed_first_key_arr](int i) {
+                    if (typed_first_key_arr->IsNull(i)) {
+                      return hash_relation_->GetNull();
+                    } else {
+                      return hash_relation_->Get(typed_key_array->GetView(i),
+                                                 typed_first_key_arr->GetString(i));
+                    }
+                  };
+                }
               }
             } break;
             default: {
@@ -1001,6 +1010,7 @@ class ConditionedProbeKernel::Impl {
       uint64_t Evaluate(std::shared_ptr<arrow::Array> key_array,
                         const arrow::ArrayVector& key_payloads) override {
         auto typed_key_array = std::dynamic_pointer_cast<arrow::Int32Array>(key_array);
+        assert(typed_key_array != nullptr);
         std::vector<std::shared_ptr<UnsafeArray>> payloads;
         int i = 0;
         bool do_unsafe_row = true;
@@ -1071,7 +1081,9 @@ class ConditionedProbeKernel::Impl {
           if (!do_unsafe_row) {
             index = fast_probe(i);
           } else {
-            unsafe_key_row->reset();
+            if (unsafe_key_row) {
+              unsafe_key_row->reset();
+            }
             for (auto payload_arr : payloads) {
               payload_arr->Append(i, &unsafe_key_row);
             }
@@ -1853,6 +1865,7 @@ ConditionedProbeKernel::ConditionedProbeKernel(
                        right_schema_list, condition, join_type, result_schema,
                        hash_configuration_list, hash_relation_idx));
   kernel_name_ = "ConditionedProbeKernel";
+  ctx_ = nullptr;
 }
 
 arrow::Status ConditionedProbeKernel::MakeResultIterator(

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/expression_codegen_visitor.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/expression_codegen_visitor.cc
@@ -1026,8 +1026,8 @@ arrow::Status ExpressionCodegenVisitor::Visit(const gandiva::BooleanNode& node)
       field_type_ = mixed;
     }
     for (auto header : child_visitor->GetHeaders()) {
-      if (std::find(header_list_.begin(), header_list_.end(), header) ==
-          header_list_.end()) {
+      if (!header.empty() && std::find(header_list_.begin(), header_list_.end(),
+                                       header) == header_list_.end()) {
         header_list_.push_back(header);
       }
     }

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/hash_aggregate_kernel.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/hash_aggregate_kernel.cc
@@ -129,6 +129,7 @@ class HashAggregateKernel::Impl {
       if (no_result_project) return;
     }
     result_expr_list_ = result_expr_node_list;
+    pool_ = nullptr;
   }
 
   virtual arrow::Status MakeResultIterator(
@@ -1113,6 +1114,7 @@ HashAggregateKernel::HashAggregateKernel(
   impl_.reset(new Impl(ctx, input_field_list, action_list, result_field_node_list,
                        result_expr_node_list));
   kernel_name_ = "HashAggregateKernelKernel";
+  ctx_ = ctx;
 }
 #undef PROCESS_SUPPORTED_TYPES
 

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/hash_relation_kernel.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/hash_relation_kernel.cc
@@ -54,6 +54,7 @@ class HashRelationKernel::Impl {
        std::shared_ptr<gandiva::Node> root_node,
        const std::vector<std::shared_ptr<arrow::Field>>& output_field_list)
       : ctx_(ctx), input_field_list_(input_field_list) {
+    pool_ = ctx_->memory_pool();
     std::vector<std::shared_ptr<HashRelationColumn>> hash_relation_list;
     for (auto field : input_field_list) {
       std::shared_ptr<HashRelationColumn> hash_relation_column;
@@ -129,7 +130,11 @@ class HashRelationKernel::Impl {
         // If single key case, we can put key in KeyArray
         auto key_type = std::dynamic_pointer_cast<arrow::FixedWidthType>(
             key_hash_field_list[0]->type());
-        key_size_ = key_type->bit_width() / 8;
+        if (key_type) {
+          key_size_ = key_type->bit_width() / 8;
+        } else {
+          key_size_ = 0;
+        }
         hash_relation_ =
             std::make_shared<HashRelation>(ctx_, hash_relation_list, key_size_);
       } else {

diff --git a/native-sql-engine/cpp/src/codegen/arrow_compute/ext/kernels_ext.cc b/native-sql-engine/cpp/src/codegen/arrow_compute/ext/kernels_ext.cc
@@ -195,6 +195,7 @@ class HashArrayKernel::Impl {
       }
       index++;
     }
+    assert(func_node_list.size() > 0);
     expr = gandiva::TreeExprBuilder::MakeExpression(func_node_list[0],
                                                     arrow::field("res", arrow::int64()));
 #ifdef DEBUG
@@ -307,6 +308,7 @@ ConcatArrayKernel::ConcatArrayKernel(
     std::vector<std::shared_ptr<arrow::DataType>> type_list) {
   impl_.reset(new Impl(ctx, type_list));
   kernel_name_ = "ConcatArrayKernel";
+  ctx_ = nullptr;
 }
 
 arrow::Status ConcatArrayKernel::Evaluate(const ArrayList& in,
@@ -323,6 +325,7 @@ class CachedRelationKernel::Impl {
         result_schema_(result_schema),
         key_field_list_(key_field_list),
         result_type_(result_type) {
+    pool_ = ctx_->memory_pool();
     for (auto field : key_field_list) {
       auto indices = result_schema->GetAllFieldIndices(field->name());
       if (indices.size() != 1) {