Skip to content

Commit

Permalink
ESQL: Simplify TableIdentifier class + rename to IndexPattern (#120797)
Browse files Browse the repository at this point in the history
This class is confusing: - It contains an **unused** `cluster` attribute
- we never separate out the cluster, it remains in the `index` field.
Also, in the constructor, this field is called `catalog`, which is a
concept entirely absent from ESQL at the moment. - It can refer to
multiple indices, even multiple wildcard patterns, but doesn't mention
this neither in its name nor javadoc. - It has little to do with tables,
which is likely a remnant of this class' usage in SQL, before the
`esql.core` split.

This PR removes the `cluster` attribute, renames the class to
`IndexPattern`, and adds javadoc to clarify that it can also contain
stuff like `remote1:idx1,remote-*:idx-*`.
  • Loading branch information
alex-spies authored Jan 24, 2025
1 parent 10e96bd commit 060c833
Show file tree
Hide file tree
Showing 14 changed files with 120 additions and 135 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
import org.elasticsearch.xpack.esql.index.EsIndex;
import org.elasticsearch.xpack.esql.index.IndexResolution;
import org.elasticsearch.xpack.esql.parser.ParsingException;
import org.elasticsearch.xpack.esql.plan.TableIdentifier;
import org.elasticsearch.xpack.esql.plan.IndexPattern;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.Drop;
import org.elasticsearch.xpack.esql.plan.logical.Enrich;
Expand Down Expand Up @@ -202,7 +202,9 @@ private static class ResolveTable extends ParameterizedAnalyzerRule<UnresolvedRe
protected LogicalPlan rule(UnresolvedRelation plan, AnalyzerContext context) {
return resolveIndex(
plan,
plan.indexMode().equals(IndexMode.LOOKUP) ? context.lookupResolution().get(plan.table().index()) : context.indexResolution()
plan.indexMode().equals(IndexMode.LOOKUP)
? context.lookupResolution().get(plan.indexPattern().indexPattern())
: context.indexResolution()
);
}

Expand All @@ -213,20 +215,20 @@ private LogicalPlan resolveIndex(UnresolvedRelation plan, IndexResolution indexR
? plan
: new UnresolvedRelation(
plan.source(),
plan.table(),
plan.indexPattern(),
plan.frozen(),
plan.metadataFields(),
plan.indexMode(),
indexResolutionMessage,
plan.commandName()
);
}
TableIdentifier table = plan.table();
if (indexResolution.matches(table.index()) == false) {
IndexPattern table = plan.indexPattern();
if (indexResolution.matches(table.indexPattern()) == false) {
// TODO: fix this (and tests), or drop check (seems SQL-inherited, where's also defective)
new UnresolvedRelation(
plan.source(),
plan.table(),
plan.indexPattern(),
plan.frozen(),
plan.metadataFields(),
plan.indexMode(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ protected PreAnalysis doPreAnalyze(LogicalPlan plan) {

plan.forEachUp(UnresolvedRelation.class, p -> {
List<TableInfo> list = p.indexMode() == IndexMode.LOOKUP ? lookupIndices : indices;
list.add(new TableInfo(p.table()));
list.add(new TableInfo(p.indexPattern()));
});
plan.forEachUp(Enrich.class, unresolvedEnriches::add);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@

package org.elasticsearch.xpack.esql.analysis;

import org.elasticsearch.xpack.esql.plan.TableIdentifier;
import org.elasticsearch.xpack.esql.plan.IndexPattern;

public class TableInfo {

private final TableIdentifier id;
private final IndexPattern id;

public TableInfo(TableIdentifier id) {
public TableInfo(IndexPattern id) {
this.id = id;
}

public TableIdentifier id() {
public IndexPattern id() {
return id;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
import org.elasticsearch.xpack.esql.expression.Order;
import org.elasticsearch.xpack.esql.expression.UnresolvedNamePattern;
import org.elasticsearch.xpack.esql.expression.function.UnresolvedFunction;
import org.elasticsearch.xpack.esql.plan.TableIdentifier;
import org.elasticsearch.xpack.esql.plan.IndexPattern;
import org.elasticsearch.xpack.esql.plan.logical.Aggregate;
import org.elasticsearch.xpack.esql.plan.logical.Dissect;
import org.elasticsearch.xpack.esql.plan.logical.Drop;
Expand Down Expand Up @@ -255,7 +255,7 @@ public LogicalPlan visitRowCommand(EsqlBaseParser.RowCommandContext ctx) {
@Override
public LogicalPlan visitFromCommand(EsqlBaseParser.FromCommandContext ctx) {
Source source = source(ctx);
TableIdentifier table = new TableIdentifier(source, null, visitIndexPattern(ctx.indexPattern()));
IndexPattern table = new IndexPattern(source, visitIndexPattern(ctx.indexPattern()));
Map<String, Attribute> metadataMap = new LinkedHashMap<>();
if (ctx.metadata() != null) {
for (var c : ctx.metadata().UNQUOTED_SOURCE()) {
Expand Down Expand Up @@ -468,7 +468,7 @@ public LogicalPlan visitMetricsCommand(EsqlBaseParser.MetricsCommandContext ctx)
throw new IllegalArgumentException("METRICS command currently requires a snapshot build");
}
Source source = source(ctx);
TableIdentifier table = new TableIdentifier(source, null, visitIndexPattern(ctx.indexPattern()));
IndexPattern table = new IndexPattern(source, visitIndexPattern(ctx.indexPattern()));

if (ctx.aggregates == null && ctx.grouping == null) {
return new UnresolvedRelation(source, table, false, List.of(), IndexMode.STANDARD, null, "METRICS");
Expand Down Expand Up @@ -530,7 +530,7 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {

UnresolvedRelation right = new UnresolvedRelation(
source(target),
new TableIdentifier(source(target.index), null, rightPattern),
new IndexPattern(source(target.index), rightPattern),
false,
emptyList(),
IndexMode.LOOKUP,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
package org.elasticsearch.xpack.esql.plan;

import org.elasticsearch.xpack.esql.core.tree.Source;

import java.util.Objects;

/**
* Contains an index pattern together with its {@link Source}. Can also be a comma-separated list, like {@code idx-*,remote:other-idx*}.
*/
public class IndexPattern {

private final Source source;
private final String indexPattern;

public IndexPattern(Source source, String indexPattern) {
this.source = source;
this.indexPattern = indexPattern;
}

public String indexPattern() {
return indexPattern;
}

@Override
public int hashCode() {
return Objects.hash(indexPattern);
}

@Override
public boolean equals(Object obj) {
if (this == obj) {
return true;
}

if (obj == null || getClass() != obj.getClass()) {
return false;
}

IndexPattern other = (IndexPattern) obj;
return Objects.equals(indexPattern, other.indexPattern);
}

public Source source() {
return source;
}

@Override
public String toString() {
return indexPattern;
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import org.elasticsearch.xpack.esql.core.expression.Attribute;
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
import org.elasticsearch.xpack.esql.core.tree.Source;
import org.elasticsearch.xpack.esql.plan.TableIdentifier;
import org.elasticsearch.xpack.esql.plan.IndexPattern;

import java.util.Collections;
import java.util.List;
Expand All @@ -22,7 +22,7 @@

public class UnresolvedRelation extends LeafPlan implements Unresolvable {

private final TableIdentifier table;
private final IndexPattern indexPattern;
private final boolean frozen;
private final List<Attribute> metadataFields;
/*
Expand All @@ -40,19 +40,19 @@ public class UnresolvedRelation extends LeafPlan implements Unresolvable {

public UnresolvedRelation(
Source source,
TableIdentifier table,
IndexPattern indexPattern,
boolean frozen,
List<Attribute> metadataFields,
IndexMode indexMode,
String unresolvedMessage,
String commandName
) {
super(source);
this.table = table;
this.indexPattern = indexPattern;
this.frozen = frozen;
this.metadataFields = metadataFields;
this.indexMode = indexMode;
this.unresolvedMsg = unresolvedMessage == null ? "Unknown index [" + table.index() + "]" : unresolvedMessage;
this.unresolvedMsg = unresolvedMessage == null ? "Unknown index [" + indexPattern.indexPattern() + "]" : unresolvedMessage;
this.commandName = commandName;
}

Expand All @@ -68,11 +68,11 @@ public String getWriteableName() {

@Override
protected NodeInfo<UnresolvedRelation> info() {
return NodeInfo.create(this, UnresolvedRelation::new, table, frozen, metadataFields, indexMode, unresolvedMsg, commandName);
return NodeInfo.create(this, UnresolvedRelation::new, indexPattern, frozen, metadataFields, indexMode, unresolvedMsg, commandName);
}

public TableIdentifier table() {
return table;
public IndexPattern indexPattern() {
return indexPattern;
}

public boolean frozen() {
Expand Down Expand Up @@ -124,7 +124,7 @@ public String unresolvedMessage() {

@Override
public int hashCode() {
return Objects.hash(source(), table, metadataFields, indexMode, unresolvedMsg);
return Objects.hash(source(), indexPattern, metadataFields, indexMode, unresolvedMsg);
}

@Override
Expand All @@ -138,7 +138,7 @@ public boolean equals(Object obj) {
}

UnresolvedRelation other = (UnresolvedRelation) obj;
return Objects.equals(table, other.table)
return Objects.equals(indexPattern, other.indexPattern)
&& Objects.equals(frozen, other.frozen)
&& Objects.equals(metadataFields, other.metadataFields)
&& indexMode == other.indexMode
Expand All @@ -147,11 +147,11 @@ public boolean equals(Object obj) {

@Override
public List<Object> nodeProperties() {
return singletonList(table);
return singletonList(indexPattern);
}

@Override
public String toString() {
return UNRESOLVED_PREFIX + table.index();
return UNRESOLVED_PREFIX + indexPattern.indexPattern();
}
}
Loading

0 comments on commit 060c833

Please sign in to comment.