(opensearch-project#1506) Remove reservedSymbolTable and replace with…

… HIDDEN_FIELD_NAME (opensearch-project#1936) * (opensearch-project#1506) Remove reservedSymbolTable and replace with HIDDEN_FIELD_NAME (#323) * opensearch-project#1506: Remove reservedSymbolTable and use HIDDEN_FIELD_NAME instead Signed-off-by: acarbonetto <[email protected]> * opensearch-project#1506: Remove reservedSymbolTable and use HIDDEN_FIELD_NAME instead Signed-off-by: acarbonetto <[email protected]> * opensearch-project#1506: Fix checkstyle errors Signed-off-by: acarbonetto <[email protected]> --------- Signed-off-by: acarbonetto <[email protected]> * opensearch-project#1506: spotless apply Signed-off-by: acarbonetto <[email protected]> --------- Signed-off-by: acarbonetto <[email protected]> (cherry picked from commit 5381a6f) Signed-off-by: acarbonetto <[email protected]>
Bit-Quill · Aug 15, 2023 · 371c0bf · 371c0bf
1 parent 3683b6c
commit 371c0bf
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 25 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
@@ -154,9 +154,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.addReservedWord(new Symbol(Namespace.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.
@@ -200,12 +200,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.addReservedWord(new Symbol(Namespace.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
@@ -362,10 +362,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> exprType = typeEnv.getReservedSymbolTable().lookup(
-            new Symbol(Namespace.FIELD_NAME, part));
+          typeEnv != null;
+          typeEnv = typeEnv.getParent()) {
+        Optional<ExprType> 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
@@ -29,9 +29,6 @@ public class TypeEnvironment implements Environment<Symbol, ExprType> {
   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<Symbol, ExprType> {
   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();
   }
 
   /**
@@ -122,8 +117,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
@@ -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
@@ -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),