[GitHub] [arrow] praveenbingo commented on a change in pull request #7495: ARROW-9185: [Java][Gandiva] Make llvm build optimisation configurable from java

[GitBox <gi...@apache.org>]

praveenbingo commented on a change in pull request #7495:
URL: https://github.com/apache/arrow/pull/7495#discussion_r443353079



##########
File path: cpp/src/gandiva/configuration.h
##########
@@ -53,7 +55,12 @@ class GANDIVA_EXPORT Configuration {
 class GANDIVA_EXPORT ConfigurationBuilder {
  public:
   std::shared_ptr<Configuration> build() {
-    std::shared_ptr<Configuration> configuration(new Configuration());
+    std::shared_ptr<Configuration> configuration(new Configuration(true));

Review comment:
       is this required..default seems to be true..

##########
File path: cpp/src/gandiva/configuration.h
##########
@@ -35,6 +35,8 @@ class GANDIVA_EXPORT Configuration {
  public:
   friend class ConfigurationBuilder;
 
+  explicit Configuration(bool optimize) : optimize_(optimize) {}

Review comment:
       also have a default flag i guess (and remove the default below)..to make it consistent for other users (if any)

##########
File path: cpp/src/gandiva/jni/config_builder.cc
##########
@@ -33,9 +33,9 @@ using gandiva::ConfigurationBuilder;
  */
 JNIEXPORT jlong JNICALL
 Java_org_apache_arrow_gandiva_evaluator_ConfigurationBuilder_buildConfigInstance(
-    JNIEnv* env, jobject configuration) {
+    JNIEnv* env, jobject configuration, jboolean optimize) {

Review comment:
       is this not backward incompatible for other users..
   
   would it be better to introduce a overloaded method and leave the old one as true?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org