From b1d43332f54c1bfdfdae3455322df1cce486820a Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 12:47:44 +0200 Subject: [PATCH 1/6] Lazily compute MvExpandExec's output --- .../xpack/esql/plan/physical/MvExpandExec.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java index ebf7d1aba7b8a..e71381b02eb06 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java @@ -20,13 +20,13 @@ public class MvExpandExec extends UnaryExec { private final NamedExpression target; private final Attribute expanded; - private final List output; + private List lazyOutput; public MvExpandExec(Source source, PhysicalPlan child, NamedExpression target, Attribute expanded) { super(source, child); this.target = target; this.expanded = expanded; - this.output = calculateOutput(child.output(), target, expanded); + this.lazyOutput = null; } @Override @@ -49,7 +49,10 @@ public Attribute expanded() { @Override public List output() { - return output; + if (lazyOutput == null) { + lazyOutput = calculateOutput(child().output(), target, expanded); + } + return lazyOutput; } @Override From 1519da757a6f876db12da08d5790492d8b5606d1 Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 13:23:56 +0200 Subject: [PATCH 2/6] Make Rename.output() correct --- .../xpack/esql/core/rule/Rule.java | 4 +-- .../xpack/esql/analysis/Analyzer.java | 28 +++++++++++++++---- .../xpack/esql/plan/logical/Rename.java | 9 ++++-- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/rule/Rule.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/rule/Rule.java index 6121c9b36442b..163b1f89f2abb 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/rule/Rule.java +++ b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/rule/Rule.java @@ -6,8 +6,8 @@ */ package org.elasticsearch.xpack.esql.core.rule; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; +import org.elasticsearch.logging.LogManager; +import org.elasticsearch.logging.Logger; import org.elasticsearch.xpack.esql.core.tree.Node; import org.elasticsearch.xpack.esql.core.util.ReflectionUtils; diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index a691f88f29f99..aecadf3b3da5b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -10,6 +10,7 @@ import org.elasticsearch.common.logging.HeaderWarning; import org.elasticsearch.common.logging.LoggerMessageFormat; import org.elasticsearch.compute.data.Block; +import org.elasticsearch.logging.Logger; import org.elasticsearch.xpack.core.enrich.EnrichPolicy; import org.elasticsearch.xpack.esql.Column; import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; @@ -383,7 +384,7 @@ private LocalRelation tableMapAsRelation(Source source, Map mapT } } - private static class ResolveRefs extends BaseAnalyzerRule { + public static class ResolveRefs extends BaseAnalyzerRule { @Override protected LogicalPlan doRule(LogicalPlan plan) { if (plan.childrenResolved() == false) { @@ -581,14 +582,25 @@ private Attribute maybeResolveAttribute(UnresolvedAttribute ua, List return resolveAttribute(ua, childrenOutput); } + private static Attribute maybeResolveAttribute(UnresolvedAttribute ua, List childrenOutput, Logger logger) { + if (ua.customMessage()) { + return ua; + } + return resolveAttribute(ua, childrenOutput, logger); + } + private Attribute resolveAttribute(UnresolvedAttribute ua, List childrenOutput) { + return resolveAttribute(ua, childrenOutput, log); + } + + private static Attribute resolveAttribute(UnresolvedAttribute ua, List childrenOutput, Logger logger) { Attribute resolved = ua; var named = resolveAgainstList(ua, childrenOutput); // if resolved, return it; otherwise keep it in place to be resolved later if (named.size() == 1) { resolved = named.get(0); - if (log.isTraceEnabled() && resolved.resolved()) { - log.trace("Resolved {} to {}", ua, resolved); + if (logger != null && logger.isTraceEnabled() && resolved.resolved()) { + logger.trace("Resolved {} to {}", ua, resolved); } } else { if (named.size() > 0) { @@ -724,6 +736,12 @@ private LogicalPlan resolveDrop(Drop drop, List childOutput) { } private LogicalPlan resolveRename(Rename rename, List childrenOutput) { + List projections = projectionsForRename(rename, childrenOutput, log); + + return new EsqlProject(rename.source(), rename.child(), projections); + } + + public static List projectionsForRename(Rename rename, List childrenOutput, Logger logger) { List projections = new ArrayList<>(childrenOutput); int renamingsCount = rename.renamings().size(); @@ -736,7 +754,7 @@ private LogicalPlan resolveRename(Rename rename, List childrenOutput) // remove attributes overwritten by a renaming: `| keep a, b, c | rename a as b` projections.removeIf(x -> x.name().equals(alias.name())); - var resolved = maybeResolveAttribute(ua, childrenOutput); + var resolved = maybeResolveAttribute(ua, childrenOutput, logger); if (resolved instanceof UnsupportedAttribute || resolved.resolved()) { var realiased = (NamedExpression) alias.replaceChildren(List.of(resolved)); projections.replaceAll(x -> x.equals(resolved) ? realiased : x); @@ -779,7 +797,7 @@ private LogicalPlan resolveRename(Rename rename, List childrenOutput) // add unresolved renamings to later trip the Verifier. projections.addAll(unresolved); - return new EsqlProject(rename.source(), rename.child(), projections); + return projections; } private LogicalPlan resolveEnrich(Enrich enrich, List childrenOutput) { diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java index f0b38d281474e..fe89d98844878 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java @@ -7,8 +7,11 @@ package org.elasticsearch.xpack.esql.plan.logical; +import org.elasticsearch.xpack.esql.analysis.Analyzer.ResolveRefs; import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Attribute; +import org.elasticsearch.xpack.esql.core.expression.Expressions; +import org.elasticsearch.xpack.esql.core.expression.NamedExpression; import org.elasticsearch.xpack.esql.core.tree.NodeInfo; import org.elasticsearch.xpack.esql.core.tree.Source; import org.elasticsearch.xpack.esql.expression.function.UnsupportedAttribute; @@ -31,8 +34,10 @@ public List renamings() { @Override public List output() { - // Rename is mapped to a Project during analysis; we do not compute the output here. - throw new IllegalStateException("Should never reach here."); + // Normally shouldn't reach here, as Rename only exists before resolution. + List projectionsAfterResolution = ResolveRefs.projectionsForRename(this, this.output(), null); + + return Expressions.asAttributes(projectionsAfterResolution); } @Override From 6bf131ae6b04578bbcc7a96bf8cb1ee434e954cf Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 13:24:06 +0200 Subject: [PATCH 3/6] Revert "Lazily compute MvExpandExec's output" This reverts commit b1d43332f54c1bfdfdae3455322df1cce486820a. --- .../xpack/esql/plan/physical/MvExpandExec.java | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java index e71381b02eb06..ebf7d1aba7b8a 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/MvExpandExec.java @@ -20,13 +20,13 @@ public class MvExpandExec extends UnaryExec { private final NamedExpression target; private final Attribute expanded; - private List lazyOutput; + private final List output; public MvExpandExec(Source source, PhysicalPlan child, NamedExpression target, Attribute expanded) { super(source, child); this.target = target; this.expanded = expanded; - this.lazyOutput = null; + this.output = calculateOutput(child.output(), target, expanded); } @Override @@ -49,10 +49,7 @@ public Attribute expanded() { @Override public List output() { - if (lazyOutput == null) { - lazyOutput = calculateOutput(child().output(), target, expanded); - } - return lazyOutput; + return output; } @Override From 85794868c4ed479cfe5ea23ebc66edd24dfce39e Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 13:28:09 +0200 Subject: [PATCH 4/6] Redirect non-static method to static method --- .../java/org/elasticsearch/xpack/esql/analysis/Analyzer.java | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index aecadf3b3da5b..3340944201934 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -576,10 +576,7 @@ private LogicalPlan resolveLookup(Lookup l, List childrenOutput) { } private Attribute maybeResolveAttribute(UnresolvedAttribute ua, List childrenOutput) { - if (ua.customMessage()) { - return ua; - } - return resolveAttribute(ua, childrenOutput); + maybeResolveAttribute(ua, childrenOutput, log); } private static Attribute maybeResolveAttribute(UnresolvedAttribute ua, List childrenOutput, Logger logger) { From 66ce6ad36bde1bce5d23fe2f18bf40f27d897f9f Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 13:36:00 +0200 Subject: [PATCH 5/6] D'oh --- .../java/org/elasticsearch/xpack/esql/analysis/Analyzer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java index 3340944201934..0fec74bf5d7c6 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java @@ -576,7 +576,7 @@ private LogicalPlan resolveLookup(Lookup l, List childrenOutput) { } private Attribute maybeResolveAttribute(UnresolvedAttribute ua, List childrenOutput) { - maybeResolveAttribute(ua, childrenOutput, log); + return maybeResolveAttribute(ua, childrenOutput, log); } private static Attribute maybeResolveAttribute(UnresolvedAttribute ua, List childrenOutput, Logger logger) { From f956dfd21fd2b9cd00cf14cf0b67ee37d274481a Mon Sep 17 00:00:00 2001 From: Alexander Spies Date: Wed, 17 Jul 2024 14:28:16 +0200 Subject: [PATCH 6/6] Another d'oh --- .../java/org/elasticsearch/xpack/esql/plan/logical/Rename.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java index fe89d98844878..29ee7f0504c70 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/Rename.java @@ -35,7 +35,7 @@ public List renamings() { @Override public List output() { // Normally shouldn't reach here, as Rename only exists before resolution. - List projectionsAfterResolution = ResolveRefs.projectionsForRename(this, this.output(), null); + List projectionsAfterResolution = ResolveRefs.projectionsForRename(this, this.child().output(), null); return Expressions.asAttributes(projectionsAfterResolution); }