From 82aaaa4abaa78a551fbd037ab17b106fd037c878 Mon Sep 17 00:00:00 2001 From: Andrew Carbonetto Date: Fri, 4 Aug 2023 13:23:32 -0700 Subject: [PATCH 1/2] (#1506) Remove reservedSymbolTable and replace with HIDDEN_FIELD_NAME (#323) * #1506: Remove reservedSymbolTable and use HIDDEN_FIELD_NAME instead Signed-off-by: acarbonetto * #1506: Remove reservedSymbolTable and use HIDDEN_FIELD_NAME instead Signed-off-by: acarbonetto * #1506: Fix checkstyle errors Signed-off-by: acarbonetto --------- Signed-off-by: acarbonetto --- .../main/java/org/opensearch/sql/analysis/Analyzer.java | 4 ++-- .../org/opensearch/sql/analysis/ExpressionAnalyzer.java | 4 ++-- .../org/opensearch/sql/analysis/TypeEnvironment.java | 9 --------- .../org/opensearch/sql/analysis/symbol/Namespace.java | 1 + .../opensearch/sql/analysis/ExpressionAnalyzerTest.java | 6 +++--- 5 files changed, 8 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index 370dd1a3f1..3df8325e48 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -161,7 +161,7 @@ public LogicalPlan visitRelation(Relation node, AnalysisContext context) { } table.getFieldTypes().forEach((k, v) -> curEnv.define(new Symbol(Namespace.FIELD_NAME, k), v)); table.getReservedFieldTypes().forEach( - (k, v) -> curEnv.addReservedWord(new Symbol(Namespace.FIELD_NAME, k), v) + (k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v) ); // Put index name or its alias in index namespace on type environment so qualifier @@ -207,7 +207,7 @@ public LogicalPlan visitTableFunction(TableFunction node, AnalysisContext contex Table table = tableFunctionImplementation.applyArguments(); table.getFieldTypes().forEach((k, v) -> curEnv.define(new Symbol(Namespace.FIELD_NAME, k), v)); table.getReservedFieldTypes().forEach( - (k, v) -> curEnv.addReservedWord(new Symbol(Namespace.FIELD_NAME, k), v) + (k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v) ); curEnv.define(new Symbol(Namespace.INDEX_NAME, dataSourceSchemaIdentifierNameResolver.getIdentifierName()), STRUCT); diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index 60e5b40a82..e4eb24804f 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -372,8 +372,8 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context for (TypeEnvironment typeEnv = context.peek(); typeEnv != null; typeEnv = typeEnv.getParent()) { - Optional exprType = typeEnv.getReservedSymbolTable().lookup( - new Symbol(Namespace.FIELD_NAME, part)); + Optional exprType = Optional.ofNullable( + typeEnv.lookupAllFields(Namespace.HIDDEN_FIELD_NAME).get(part)); if (exprType.isPresent()) { return visitMetadata( qualifierAnalyzer.unqualified(node), diff --git a/core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java b/core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java index 17d203f66f..8032cba706 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java +++ b/core/src/main/java/org/opensearch/sql/analysis/TypeEnvironment.java @@ -29,9 +29,6 @@ public class TypeEnvironment implements Environment { private final TypeEnvironment parent; private final SymbolTable symbolTable; - @Getter - private final SymbolTable reservedSymbolTable; - /** * Constructor with empty symbol tables. * @@ -40,7 +37,6 @@ public class TypeEnvironment implements Environment { public TypeEnvironment(TypeEnvironment parent) { this.parent = parent; this.symbolTable = new SymbolTable(); - this.reservedSymbolTable = new SymbolTable(); } /** @@ -52,7 +48,6 @@ public TypeEnvironment(TypeEnvironment parent) { public TypeEnvironment(TypeEnvironment parent, SymbolTable symbolTable) { this.parent = parent; this.symbolTable = symbolTable; - this.reservedSymbolTable = new SymbolTable(); } /** @@ -133,8 +128,4 @@ public void clearAllFields() { lookupAllFields(FIELD_NAME).keySet().forEach( v -> remove(new Symbol(Namespace.FIELD_NAME, v))); } - - public void addReservedWord(Symbol symbol, ExprType type) { - reservedSymbolTable.store(symbol, type); - } } diff --git a/core/src/main/java/org/opensearch/sql/analysis/symbol/Namespace.java b/core/src/main/java/org/opensearch/sql/analysis/symbol/Namespace.java index b5203033a8..32cb9dee35 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/symbol/Namespace.java +++ b/core/src/main/java/org/opensearch/sql/analysis/symbol/Namespace.java @@ -13,6 +13,7 @@ public enum Namespace { INDEX_NAME("Index"), FIELD_NAME("Field"), + HIDDEN_FIELD_NAME("HiddenField"), FUNCTION_NAME("Function"); private final String name; diff --git a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java index 5a05c79132..30a43f557f 100644 --- a/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java +++ b/core/src/test/java/org/opensearch/sql/analysis/ExpressionAnalyzerTest.java @@ -234,8 +234,8 @@ public void qualified_name_with_qualifier() { public void qualified_name_with_reserved_symbol() { analysisContext.push(); - analysisContext.peek().addReservedWord(new Symbol(Namespace.FIELD_NAME, "_reserved"), STRING); - analysisContext.peek().addReservedWord(new Symbol(Namespace.FIELD_NAME, "_priority"), FLOAT); + analysisContext.peek().define(new Symbol(Namespace.HIDDEN_FIELD_NAME, "_reserved"), STRING); + analysisContext.peek().define(new Symbol(Namespace.HIDDEN_FIELD_NAME, "_priority"), FLOAT); analysisContext.peek().define(new Symbol(Namespace.INDEX_NAME, "index_alias"), STRUCT); assertAnalyzeEqual( DSL.ref("_priority", FLOAT), @@ -246,7 +246,7 @@ public void qualified_name_with_reserved_symbol() { qualifiedName("index_alias", "_reserved") ); - // reserved fields take priority over symbol table + // cannot replace an existing field type analysisContext.peek().define(new Symbol(Namespace.FIELD_NAME, "_reserved"), LONG); assertAnalyzeEqual( DSL.ref("_reserved", STRING), From b410e8d8a61a545135fc9969faf1da13c80c4a36 Mon Sep 17 00:00:00 2001 From: acarbonetto Date: Mon, 14 Aug 2023 10:30:14 -0700 Subject: [PATCH 2/2] #1506: spotless apply Signed-off-by: acarbonetto --- .../org/opensearch/sql/analysis/Analyzer.java | 21 +++++++++++-------- .../sql/analysis/ExpressionAnalyzer.java | 8 +++---- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java index ff736ad226..d5e8b93b13 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/Analyzer.java @@ -159,9 +159,9 @@ public LogicalPlan visitRelation(Relation node, AnalysisContext context) { dataSourceSchemaIdentifierNameResolver.getIdentifierName()); } table.getFieldTypes().forEach((k, v) -> curEnv.define(new Symbol(Namespace.FIELD_NAME, k), v)); - table.getReservedFieldTypes().forEach( - (k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v) - ); + table + .getReservedFieldTypes() + .forEach((k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v)); // Put index name or its alias in index namespace on type environment so qualifier // can be removed when analyzing qualified name. The value (expr type) here doesn't matter. @@ -213,12 +213,15 @@ public LogicalPlan visitTableFunction(TableFunction node, AnalysisContext contex TypeEnvironment curEnv = context.peek(); Table table = tableFunctionImplementation.applyArguments(); table.getFieldTypes().forEach((k, v) -> curEnv.define(new Symbol(Namespace.FIELD_NAME, k), v)); - table.getReservedFieldTypes().forEach( - (k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v) - ); - curEnv.define(new Symbol(Namespace.INDEX_NAME, - dataSourceSchemaIdentifierNameResolver.getIdentifierName()), STRUCT); - return new LogicalRelation(dataSourceSchemaIdentifierNameResolver.getIdentifierName(), + table + .getReservedFieldTypes() + .forEach((k, v) -> curEnv.define(new Symbol(Namespace.HIDDEN_FIELD_NAME, k), v)); + curEnv.define( + new Symbol( + Namespace.INDEX_NAME, dataSourceSchemaIdentifierNameResolver.getIdentifierName()), + STRUCT); + return new LogicalRelation( + dataSourceSchemaIdentifierNameResolver.getIdentifierName(), tableFunctionImplementation.applyArguments()); } diff --git a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java index 4d23cf3aa4..5a8d6fe976 100644 --- a/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java +++ b/core/src/main/java/org/opensearch/sql/analysis/ExpressionAnalyzer.java @@ -375,10 +375,10 @@ public Expression visitQualifiedName(QualifiedName node, AnalysisContext context // check for reserved words in the identifier for (String part : node.getParts()) { for (TypeEnvironment typeEnv = context.peek(); - typeEnv != null; - typeEnv = typeEnv.getParent()) { - Optional exprType = Optional.ofNullable( - typeEnv.lookupAllFields(Namespace.HIDDEN_FIELD_NAME).get(part)); + typeEnv != null; + typeEnv = typeEnv.getParent()) { + Optional exprType = + Optional.ofNullable(typeEnv.lookupAllFields(Namespace.HIDDEN_FIELD_NAME).get(part)); if (exprType.isPresent()) { return visitMetadata( qualifierAnalyzer.unqualified(node), (ExprCoreType) exprType.get(), context);