diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/Rule.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/Rule.java index fe519ee7435..d63357e151a 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/Rule.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/Rule.java @@ -12,17 +12,14 @@ import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.xpack.sql.tree.Node; import org.elasticsearch.xpack.sql.util.ReflectionUtils; -// Rule class that applies a transformation to a tree. -// In addition, performs type filtering so that a rule that works only on a type node can be skipped if necessary just based on the class signature. - -// Implementation detail: -// While lambdas are nice, actual node rules tend to be fairly large and are much better suited as full blown classes. -// In addition as they already embed their type information (all rule implementations end up with the generic information on them) -// this can be leveraged to perform the type filtering (as mentioned above). -// As a side note, getting the generic information from lambdas is very hacky and not portable (not without completely messing the JVM SM) - -// apply - indicates how to apply the rule (transformUp/Down, transformExpressions..) on the target -// rule - contains the actual rule logic. +/** + * Rules that apply transformation to a tree. In addition, performs + * type filtering so that a rule that the rule implementation doesn't + * have to manually filter. + *

+ * Rules could could be built as lambdas but most + * rules are much larger so we keep them as full blown subclasses. + */ public abstract class Rule> implements UnaryOperator { protected Logger log = Loggers.getLogger(getClass()); @@ -52,4 +49,4 @@ public abstract class Rule> implements UnaryOpera public String toString() { return name(); } -} \ No newline at end of file +} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java index 1643cab3a84..2936e6342ad 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java @@ -68,7 +68,7 @@ public abstract class RuleExecutor> { } private final Iterable batches = batches(); - + protected abstract Iterable.Batch> batches(); public class Transformation { @@ -103,7 +103,7 @@ public abstract class RuleExecutor> { } public class ExecutionInfo { - + private final TreeType before, after; private final Map> transformations; @@ -145,12 +145,12 @@ public abstract class RuleExecutor> { boolean hasChanged = false; long batchStart = System.currentTimeMillis(); long batchDuration = 0; - + // run each batch until no change occurs or the limit is reached do { hasChanged = false; batchRuns++; - + for (Rule rule : batch.rules) { Transformation tf = new Transformation(currentPlan, rule); tfs.add(tf); @@ -168,11 +168,11 @@ public abstract class RuleExecutor> { } } } - batchDuration = System.currentTimeMillis() - batchStart; + batchDuration = System.currentTimeMillis() - batchStart; } while (hasChanged && !batch.limit.reached(batchRuns)); - + totalDuration += batchDuration; - + if (log.isTraceEnabled()) { TreeType before = plan; TreeType after = plan; @@ -180,16 +180,16 @@ public abstract class RuleExecutor> { before = tfs.get(0).before; after = tfs.get(tfs.size() - 1).after; } - log.trace("Batch {} applied took {}\n{}", batch.name, TimeValue.timeValueMillis(batchDuration), NodeUtils.diffString(before, after)); + log.trace("Batch {} applied took {}\n{}", + batch.name, TimeValue.timeValueMillis(batchDuration), NodeUtils.diffString(before, after)); } } - - if (!currentPlan.equals(plan)) { - if (log.isDebugEnabled()) { - log.debug("Tree transformation took {}\n{}", TimeValue.timeValueMillis(totalDuration), NodeUtils.diffString(plan, currentPlan)); - } + + if (false == currentPlan.equals(plan) && log.isDebugEnabled()) { + log.debug("Tree transformation took {}\n{}", + TimeValue.timeValueMillis(totalDuration), NodeUtils.diffString(plan, currentPlan)); } return new ExecutionInfo(plan, currentPlan, transformations); } -} \ No newline at end of file +}