[BUG] Underscore in identifier is ignored by ANTLR parser

[dai-chen]

What is the bug?

For some reason, underscore in a query is ignored by ANTLR parser. Take the query in our doc for example, _id becomes id after parsing. This caused no fallback happen and failure in v2 engine since metadata field is not supported yet.

How can one reproduce the bug?

PUT sql-test/_doc/1
{ 
  "name": "John",
  "age": 10
}

POST _plugins/_sql
{
  "query" : """
    SELECT * FROM sql-test WHERE _id = 1
  """
}

org.opensearch.sql.exception.SemanticCheckException: can't resolve Symbol(namespace=FIELD_NAME, name=id) in type env
	at org.opensearch.sql.analysis.TypeEnvironment.resolve(TypeEnvironment.java:57) ~[core-2.4.0.0-SNAPSHOT.jar:?]
	at org.opensearch.sql.analysis.ExpressionAnalyzer.visitIdentifier(ExpressionAnalyzer.java:323) ~[core-2.4.0.0-SNAPSHOT.jar:?]
	at org.opensearch.sql.analysis.ExpressionAnalyzer.visitQualifiedName(ExpressionAnalyzer.java:297) ~[core-2.4.0.0-SNAPSHOT.jar:?]
	at org.opensearch.sql.analysis.ExpressionAnalyzer.visitQualifiedName(ExpressionAnalyzer.java:73) ~[core-2.4.0.0-SNAPSHOT.jar:?]
	at org.opensearch.sql.ast.expression.QualifiedName.accept(QualifiedName.java:111) ~[core-2.4.0.0-SNAPSHOT.jar:?]
	at org.opensearch.sql.analysis.ExpressionAnalyzer.analyze(ExpressionAnalyzer.java:90) ~[core-2.4.0.0-SNAPSHOT.jar:?]

What is the expected behavior?

User can use _id without issue for backward compatibility.

What is your host/environment?

• Version 2.4.0

Do you have any additional context?

Not clear when is this broken. It is missed because all 5 IT on it are ignored. Because it's used in our documentation, we'd better fix it or the doc soon.

[MitchellGale]

I think these might be related issues:
#639
#783
#339

[dai-chen]

@MitchellGale-BitQuill Thanks for the info! It's strange that the example query has been used in documentation since 1.0. However, I tested it and found same problem with 1.0 docker. Need to confirm and see if we need to fix in code or in doc.

[acarbonetto]

We would need to extend the parser like: Bit-Quill#142
Not sure if that's the direction we want to take this since this doesn't apply to other storage types

[dai-chen]

We would need to extend the parser like: Bit-Quill#142 Not sure if that's the direction we want to take this since this doesn't apply to other storage types

yes, because of the backward compatibility, I think we need to support it. Otherwise users have to always back quote although it is more ANSI SQL standard. For other storages, I think it's fine and please see "hidden" columns in Presto Hive connector: https://prestodb.io/docs/current/connector/hive.html#extra-hidden-columns. Analyzer is supposed to throw exception if _id is used with S3 table because S3 storage doesn't return such column in getFieldTypes interface. The only drawback is it's semantic error rather than syntax error because we allow it in ANTLR grammar.

[acarbonetto]

Good to know. We'll push that PR forward since it solves a few bugs.
If, moving forward, we want a different solution for other storage engines, we can refactor later.

[acarbonetto]

@dai-chen this can be closed as part of #1456
We now consider any field with _ so long as the field isn't reserved by OpenSearch.
Certain reserved fields are now supported (_id, _index, _score, _sort, _maxscore).