projjal commented on code in PR #13446:
URL: https://github.com/apache/arrow/pull/13446#discussion_r940918137
##########
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/Projector.java:
##########
@@ -187,6 +187,45 @@ public static Projector make(Schema schema,
List<ExpressionTree> exprs,
public static Projector make(Schema schema, List<ExpressionTree> exprs,
SelectionVectorType selectionVectorType,
long configurationId) throws GandivaException {
Review Comment:
Can you add some java unit test..only with no-op sec cache..just to verify
the jni flow
##########
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/Projector.java:
##########
@@ -187,6 +187,45 @@ public static Projector make(Schema schema,
List<ExpressionTree> exprs,
public static Projector make(Schema schema, List<ExpressionTree> exprs,
SelectionVectorType selectionVectorType,
long configurationId) throws GandivaException {
+ return make(schema, exprs, selectionVectorType, configurationId, null);
+ }
+
+ /**
+ * Invoke this function to generate LLVM code to evaluate the list of
project expressions.
+ * Invoke Projector::Evaluate() against a RecordBatch to evaluate the record
batch
+ * against these projections.
+ *
+ * @param schema Table schema. The field names in the schema should
match the fields used
+ * to create the TreeNodes
+ * @param exprs List of expressions to be evaluated against data
+ * @param configOptions ConfigOptions parameter
+ * @param secondaryCache SecondaryCache persistant cache for gandiva object
code.
+ *
+ * @return A native evaluator object that can be used to invoke these
projections on a RecordBatch
+ */
+ public static Projector make(Schema schema, List<ExpressionTree> exprs,
+ ConfigurationBuilder.ConfigOptions configOptions,
+ JavaSecondaryCacheInterface secondaryCache) throws GandivaException {
+ return make(schema, exprs, SelectionVectorType.SV_NONE,
JniLoader.getConfiguration(configOptions), secondaryCache);
+ }
+
+ /**
+ * Invoke this function to generate LLVM code to evaluate the list of
project expressions.
+ * Invoke Projector::Evaluate() against a RecordBatch to evaluate the record
batch
+ * against these projections.
+ *
+ * @param schema Table schema. The field names in the schema
should match the fields used
+ * to create the TreeNodes
+ * @param exprs List of expressions to be evaluated against
data
+ * @param selectionVectorType type of selection vector
+ * @param configurationId Custom configuration created through config
builder.
+ * @param secondaryCache SecondaryCache persistant cache for gandiva
object code.
Review Comment:
nit: probably no need to mention persistent..The implementation may be
anything
##########
cpp/src/gandiva/engine.cc:
##########
@@ -89,6 +89,7 @@ std::once_flag llvm_init_once_flag;
static bool llvm_init = false;
static llvm::StringRef cpu_name;
static llvm::SmallVector<std::string, 10> cpu_attrs;
+static char* engine_str_;
Review Comment:
use a different name..like cpu_details...or something else
##########
java/gandiva/src/main/java/org/apache/arrow/gandiva/evaluator/JavaSecondaryCacheInterface.java:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.gandiva.evaluator;
+
+/**
+ * Acts as an interface to the secondary cache class that uses a callback
+ * mechanism from gandiva.
+ */
+public interface JavaSecondaryCacheInterface {
+ /**
+ * Resultant buffer of the get call to the secondary cache.
+ */
+ class ExpandResult {
Review Comment:
we don't do any expansion right? better rename it to BufferResult or
something.
##########
cpp/src/gandiva/engine.cc:
##########
@@ -112,9 +113,20 @@ void Engine::InitOnce() {
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+
+ std::string engine_str = cpu_name.str();
+ engine_str += cpu_attrs_str;
+ engine_str_ = strdup(engine_str.c_str());
Review Comment:
why not just store std::string variable
##########
cpp/src/gandiva/engine.cc:
##########
@@ -112,9 +113,20 @@ void Engine::InitOnce() {
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+
+ std::string engine_str = cpu_name.str();
+ engine_str += cpu_attrs_str;
+ engine_str_ = strdup(engine_str.c_str());
+
llvm_init = true;
}
+char* Engine::ToString() {
Review Comment:
also use a different name for this method..since we are only resulting the
cpu details
##########
cpp/src/gandiva/engine.cc:
##########
@@ -112,9 +113,20 @@ void Engine::InitOnce() {
}
ARROW_LOG(INFO) << "Detected CPU Name : " << cpu_name.str();
ARROW_LOG(INFO) << "Detected CPU Features:" << cpu_attrs_str;
+
+ std::string engine_str = cpu_name.str();
Review Comment:
use a different name..like cpu_details...or something
##########
cpp/src/gandiva/projector.cc:
##########
@@ -83,6 +100,21 @@ Status Projector::Make(SchemaPtr schema, const
ExpressionVector& exprs,
std::unique_ptr<LLVMGenerator> llvm_gen;
ARROW_RETURN_NOT_OK(LLVMGenerator::Make(configuration, is_cached,
&llvm_gen));
+ if (!is_cached && sec_cache != nullptr) {
+ std::string key = std::string(Engine::ToString()) + cache_key.ToString();
Review Comment:
also add a comment there why cpu details are required
##########
cpp/src/gandiva/projector.cc:
##########
@@ -83,6 +100,21 @@ Status Projector::Make(SchemaPtr schema, const
ExpressionVector& exprs,
std::unique_ptr<LLVMGenerator> llvm_gen;
ARROW_RETURN_NOT_OK(LLVMGenerator::Make(configuration, is_cached,
&llvm_gen));
+ if (!is_cached && sec_cache != nullptr) {
+ std::string key = std::string(Engine::ToString()) + cache_key.ToString();
Review Comment:
Have a separate method to get the sec cache key..which both projector and
filter calls..also maybe add a delimiter char between the engine & cache_key.
--
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]
