From 90aee54251592323af657f06640f3fc304350029 Mon Sep 17 00:00:00 2001 From: javanna Date: Mon, 4 Dec 2017 14:57:35 +0100 Subject: [PATCH] Trim down the Catalog implementations to a single one Catalog is now a final class rather than an interface with different implementations. relates elastic/x-pack-elasticsearch#3179 Original commit: elastic/x-pack-elasticsearch@4cc927e1135b1b6b99a14b3b9614123ee7646083 --- .../xpack/qa/sql/rest/RestSqlTestCase.java | 6 ++-- .../xpack/sql/analysis/analyzer/Analyzer.java | 8 ++--- .../xpack/sql/analysis/catalog/Catalog.java | 33 ++++++++++++++----- .../xpack/sql/analysis/catalog/EsIndex.java | 2 ++ .../sql/analysis/catalog/IndexResolver.java | 12 ++++--- .../analysis/catalog/PreloadedCatalog.java | 23 ------------- .../xpack/sql/session/SqlSession.java | 1 + .../analyzer/VerifierErrorMessagesTests.java | 5 +-- .../sql/analysis/catalog/InMemoryCatalog.java | 29 ---------------- 9 files changed, 41 insertions(+), 78 deletions(-) delete mode 100644 sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/PreloadedCatalog.java delete mode 100644 sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 814885e1b5b..f9c627c9dca 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java @@ -128,12 +128,12 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe } @Override - public void testSelectInvalidSql() throws Exception { + public void testSelectInvalidSql() { expectBadRequest(() -> runSql("SELECT * FRO"), containsString("1:8: Cannot determine columns for *")); } @Override - public void testSelectFromMissingIndex() throws IOException { + public void testSelectFromMissingIndex() { expectBadRequest(() -> runSql("SELECT * FROM missing"), containsString("1:15: Unknown index [missing]")); } @@ -161,7 +161,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe private void expectBadRequest(ThrowingRunnable code, Matcher errorMessageMatcher) { ResponseException e = expectThrows(ResponseException.class, code); - assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); + assertEquals(e.getMessage(), 400, e.getResponse().getStatusLine().getStatusCode()); assertThat(e.getMessage(), errorMessageMatcher); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index c316c5c0701..46aada4fa99 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java @@ -253,17 +253,13 @@ public class Analyzer extends RuleExecutor { @Override protected LogicalPlan rule(UnresolvedRelation plan) { TableIdentifier table = plan.table(); - EsIndex found = null; - GetIndexResult index = SqlSession.currentContext().catalog.getIndex(table.index()); - if (index.isValid()) { - found = index.get(); - } else { + if (index.isValid() == false) { return plan.unresolvedMessage().equals(index.toString()) ? plan : new UnresolvedRelation(plan.location(), plan.table(), plan.alias(), index.toString()); } - LogicalPlan catalogTable = new EsRelation(plan.location(), found); + LogicalPlan catalogTable = new EsRelation(plan.location(), index.get()); SubQueryAlias sa = new SubQueryAlias(plan.location(), catalogTable, table.index()); if (plan.alias() != null) { diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java index 5fc42a6ae0a..3da1c55bb57 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java @@ -5,26 +5,39 @@ */ package org.elasticsearch.xpack.sql.analysis.catalog; -import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.common.Nullable; import java.util.Objects; +import java.util.function.Function; /** - * Converts from Elasticsearch's internal metadata ({@link ClusterState}) - * into a representation that is compatible with SQL (@{link {@link EsIndex}). + * Index representation that is compatible with SQL ({@link EsIndex}). */ -public interface Catalog { +public final class Catalog { - Catalog EMPTY = GetIndexResult::notFound; + public static final Catalog EMPTY = new Catalog(GetIndexResult::notFound); + private final Function resultFunction; + + //TODO given that this always holds a single index, we cana probably get rid of the whole idea of Catalog + public Catalog(GetIndexResult result) { + assert result != null; + this.resultFunction = index -> result.matches(index) ? result : GetIndexResult.notFound(index); + } + + private Catalog(Function resultFunction) { + assert resultFunction != null; + this.resultFunction = resultFunction; + } + /** * Lookup the information for a table. */ - @Nullable - GetIndexResult getIndex(String index); + public GetIndexResult getIndex(String index) { + return resultFunction.apply(index); + } - class GetIndexResult { + public static final class GetIndexResult { public static GetIndexResult valid(EsIndex index) { Objects.requireNonNull(index, "index must not be null if it was found"); return new GetIndexResult(index, null); @@ -47,6 +60,10 @@ public interface Catalog { this.invalid = invalid; } + private boolean matches(String index) { + return isValid() && this.index.name().equals(index); + } + /** * Get the {@linkplain EsIndex} built by the {@linkplain Catalog}. * @throws MappingException if the index is invalid for diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java index 17e2b539a74..98e388f461e 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsIndex.java @@ -20,6 +20,8 @@ public class EsIndex { private final Map mapping; public EsIndex(String name, Map mapping) { + assert name != null; + assert mapping != null; this.name = name; this.mapping = mapping; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/IndexResolver.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/IndexResolver.java index 1fcef8ffe2b..8bf1d1ccec4 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/IndexResolver.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/IndexResolver.java @@ -38,10 +38,10 @@ public class IndexResolver { public void asCatalog(final String index, ActionListener listener) { GetIndexRequest getIndexRequest = createGetIndexRequest(index); client.admin().indices().getIndex(getIndexRequest, ActionListener.wrap(getIndexResponse -> { - Map results = new HashMap<>(); + GetIndexResult result; if (getIndexResponse.getMappings().size() > 1) { - results.put(index, GetIndexResult.invalid( - "[" + index + "] is an alias pointing to more than one index which is currently incompatible with sql")); + result = GetIndexResult.invalid( + "[" + index + "] is an alias pointing to more than one index which is currently incompatible with sql"); } else if (getIndexResponse.getMappings().size() == 1){ ObjectObjectCursor> indexMappings = getIndexResponse.getMappings().iterator().next(); @@ -53,9 +53,11 @@ public class IndexResolver { * make sure that the search is executed against the same alias name from the original command, rather than * the resolved concrete index that we get back from the get index API */ - results.put(index, buildGetIndexResult(concreteIndex, index, indexMappings.value)); + result = buildGetIndexResult(concreteIndex, index, indexMappings.value); + } else { + result = GetIndexResult.notFound(index); } - listener.onResponse(new PreloadedCatalog(results)); + listener.onResponse(new Catalog(result)); }, listener::onFailure)); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/PreloadedCatalog.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/PreloadedCatalog.java deleted file mode 100644 index bad29974200..00000000000 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/PreloadedCatalog.java +++ /dev/null @@ -1,23 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.analysis.catalog; - -import java.util.Map; - - -public class PreloadedCatalog implements Catalog { - - private final Map map; - - public PreloadedCatalog(Map map) { - this.map = map; - } - - @Override - public GetIndexResult getIndex(String index) { - return map.getOrDefault(index, GetIndexResult.notFound(index)); - } -} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java index 7bb8a5651ff..4a29556ed84 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/session/SqlSession.java @@ -176,6 +176,7 @@ public class SqlSession { listener.onFailure(new SqlIllegalArgumentException("Queries with multiple indices are not supported")); return; } + //TODO why do we have a list if we only support one single element? Seems like it's the wrong data structure? if (preAnalysis.indices.size() == 1) { indexResolver.asCatalog(preAnalysis.indices.get(0), wrap(c -> listener.onResponse(action.apply(c)), listener::onFailure)); diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java index 7b2a9f9b1bb..7a55129af21 100644 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java +++ b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/analyzer/VerifierErrorMessagesTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.test.junit.annotations.TestLogging; import org.elasticsearch.xpack.sql.analysis.AnalysisException; import org.elasticsearch.xpack.sql.analysis.catalog.Catalog; import org.elasticsearch.xpack.sql.analysis.catalog.EsIndex; -import org.elasticsearch.xpack.sql.analysis.catalog.InMemoryCatalog; import org.elasticsearch.xpack.sql.expression.function.DefaultFunctionRegistry; import org.elasticsearch.xpack.sql.expression.function.FunctionRegistry; import org.elasticsearch.xpack.sql.parser.SqlParser; @@ -23,8 +22,6 @@ import org.junit.Before; import java.util.LinkedHashMap; import java.util.Map; -import static java.util.Collections.singletonList; - @TestLogging("org.elasticsearch.xpack.sql:TRACE") public class VerifierErrorMessagesTests extends ESTestCase { @@ -43,7 +40,7 @@ public class VerifierErrorMessagesTests extends ESTestCase { mapping.put("text", DataTypes.TEXT); mapping.put("keyword", DataTypes.KEYWORD); EsIndex test = new EsIndex("test", mapping); - catalog = new InMemoryCatalog(singletonList(test)); + catalog = new Catalog(Catalog.GetIndexResult.valid(test)); analyzer = new Analyzer(functionRegistry); } diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java deleted file mode 100644 index 5cc5f13b704..00000000000 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java +++ /dev/null @@ -1,29 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ -package org.elasticsearch.xpack.sql.analysis.catalog; - -import java.util.List; -import java.util.Map; -import java.util.function.Function; - -import static java.util.stream.Collectors.toMap; - -/** - * In memory implementation of catalog designed for testing. - */ -public class InMemoryCatalog implements Catalog { - private final Map indices; - - public InMemoryCatalog(List indices) { - this.indices = indices.stream().collect(toMap(EsIndex::name, Function.identity())); - } - - @Override - public GetIndexResult getIndex(String index) { - EsIndex result = indices.get(index); - return result == null ? GetIndexResult.notFound(index) : GetIndexResult.valid(result); - } -} \ No newline at end of file