[GitHub] lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen optimization of SQL functions with constant values
lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen
optimization of SQL functions with constant values
URL: https://github.com/apache/drill/pull/1481#discussion_r225775240
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggTemplate.java
##
@@ -1074,7 +1078,12 @@ private void addBatchHolder(int part, int
batchRowCount) {
// These methods are overridden in the generated class when created as plain
Java code.
protected BatchHolder newBatchHolder(int batchRowCount) {
-return new BatchHolder(batchRowCount);
+return this.injectMembers(new BatchHolder(batchRowCount));
+ }
+
+ protected BatchHolder injectMembers(BatchHolder batchHolder) {
+CodeGenMemberInjector.injectMembers(cg, batchHolder, context);
Review comment:
The ClassGenerator and FragmentContext is needed in this method for general
cases, but not for test classes, see example in `ExampleTemplateWithInner.java`
newBatchHolder is override by generated code can be implemented as follows
to remove above method
`protected BatchHolder newBatchHolder(int batchRowCount) { return
CodeGenMemberInjector.injectMembers(cg, new BatchHolder(batchRowCount),
context); }`
but the `cg` and `context` params are hard coded, not flexable. I think it
is better to make a function call to hide the implementation which may differ
in each template.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen optimization of SQL functions with constant values
lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen
optimization of SQL functions with constant values
URL: https://github.com/apache/drill/pull/1481#discussion_r225770061
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/common/CodeGenMemberInjector.java
##
@@ -0,0 +1,79 @@
+/*
+ * 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.drill.exec.physical.impl.common;
+
+import com.sun.codemodel.JMethod;
+import io.netty.buffer.DrillBuf;
+import org.apache.calcite.util.Pair;
+import org.apache.drill.exec.expr.ClassGenerator;
+import org.apache.drill.exec.expr.holders.ValueHolder;
+import org.apache.drill.exec.ops.FragmentContext;
+import org.apache.drill.shaded.guava.com.google.common.base.Function;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+public class CodeGenMemberInjector {
+
+ /**
+ * Generated code for a class may have several class members, they
+ * are initialized by invoking this method when the instance created.
+ *
+ * @param cg the class generator
+ * @param instance the class instance created by the compiler
+ * @param context the fragment context
+ */
+ public static void injectMembers(ClassGenerator cg, Object instance,
FragmentContext context) {
+Map cachedInstances = new HashMap<>();
+for (Map.Entry, Function> setter : cg.getSetters().entrySet()) {
Review comment:
Sorry I don't quite follow you since the instance is created somewhere else
not in setter collection
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen optimization of SQL functions with constant values
lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen
optimization of SQL functions with constant values
URL: https://github.com/apache/drill/pull/1481#discussion_r225764315
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ClassGenerator.java
##
@@ -646,7 +687,7 @@ public void preparePlainJava() {
Class p = params[i];
childNew.arg(shim.param(model._ref(p), "arg" + i));
}
- shim.body()._return(childNew);
+ shim.body()._return(JExpr._this().invoke("injectMembers").arg(childNew));
Review comment:
The innerClasses like BatchHolder may have the constants, BatchHolder
instance is created via newBatchHolder dynamically when processing recordBatch,
that is the difference between innerClasses and nested splitting classes.
The innerClasses member fields is initialized by invoking
injectMembers(innerClassInstance) which is override generated by this code as
shown in
`protected BatchHolder newBatchHolder(int batchRowCount) {
return this.injectMembers(new BatchHolder(batchRowCount));
}`
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen optimization of SQL functions with constant values
lushuifeng commented on a change in pull request #1481: DRILL-6763: Codegen
optimization of SQL functions with constant values
URL: https://github.com/apache/drill/pull/1481#discussion_r225762550
##
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/expr/ClassGenerator.java
##
@@ -531,11 +541,42 @@ public JVar declareClassField(String prefix, JType t) {
public JVar declareClassField(String prefix, JType t, JExpression init) {
if (innerClassGenerator != null && hasMaxIndexValue()) {
- return innerClassGenerator.clazz.field(JMod.NONE, t, prefix + index++,
init);
+ return innerClassGenerator.declareClassField(prefix, t, init);
}
return clazz.field(JMod.NONE, t, prefix + index++, init);
}
+ /**
+ * declare a setter method for the argument {@code var}.
+ * argument {@code function} holds the constant value which
+ * returns a value holder must be invoked when the class instance created.
+ * the setter method of innerClassField will be invoked if
innerClassGenerator exists.
+ *
+ * @param var the class member variable
+ * @param function the function holds the constant value
+ * @return the depth of nested class, setter method
+ */
+ public Pair declareSetterMethod(JVar var,
Function function) {
+JMethod setter = clazz.method(JMod.PUBLIC, void.class, "set" +
StringUtils.capitalize(var.name()));
Review comment:
Is It better by NOT declaring the setter function, these fields can be
accessed by reflection directly, so the number of constants in constant pool
can be reduced
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
