Skip to content

Commit 7b30898

Browse files
committed
Address review feedback on table function relation grammar
1. Canonicalize argument names at parser boundary: unquoteIdentifier + toLowerCase(Locale.ROOT) in visitTableFunctionRelation so FIELD='x' and `field`='x' both produce argName="field" 2. Make AS keyword optional (AS? alias) for consistency with tableAsRelation and subqueryAsRelation grammar rules 3. Strengthen test coverage: - Full structural AST assertion for WHERE + ORDER BY + LIMIT (verifies Sort, Limit, Filter nodes, not just toString) - Argument reorder test proves names resolve by name not position - Case canonicalization test (TABLE= → table=) - Alias-without-AS test (FROM func(...) v) Signed-off-by: Eric Wei <mengwei.eric@gmail.com>
1 parent 023d31f commit 7b30898

3 files changed

Lines changed: 87 additions & 16 deletions

File tree

sql/src/main/antlr/OpenSearchSQLParser.g4

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -111,7 +111,7 @@ fromClause
111111
relation
112112
: tableName (AS? alias)? # tableAsRelation
113113
| LR_BRACKET subquery = querySpecification RR_BRACKET AS? alias # subqueryAsRelation
114-
| qualifiedName LR_BRACKET tableFunctionArgs RR_BRACKET AS alias # tableFunctionRelation
114+
| qualifiedName LR_BRACKET tableFunctionArgs RR_BRACKET AS? alias # tableFunctionRelation
115115
;
116116

117117
tableFunctionArgs

sql/src/main/java/org/opensearch/sql/sql/parser/AstBuilder.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import com.google.common.collect.ImmutableList;
2323
import java.util.Collections;
24+
import java.util.Locale;
2425
import java.util.Optional;
2526
import lombok.RequiredArgsConstructor;
2627
import org.antlr.v4.runtime.tree.ParseTree;
@@ -199,7 +200,9 @@ public UnresolvedPlan visitTableFunctionRelation(TableFunctionRelationContext ct
199200
.tableFunctionArg()
200201
.forEach(
201202
arg -> {
202-
String argName = arg.ident().getText();
203+
String argName =
204+
StringUtils.unquoteIdentifier(arg.ident().getText())
205+
.toLowerCase(Locale.ROOT);
203206
UnresolvedExpression argValue = visitAstExpression(arg.functionArg());
204207
args.add(new UnresolvedArgument(argName, argValue));
205208
});

sql/src/test/java/org/opensearch/sql/sql/parser/AstBuilderTest.java

Lines changed: 82 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,7 @@
77

88
import static java.util.Collections.emptyList;
99
import static org.junit.jupiter.api.Assertions.assertEquals;
10-
import static org.junit.jupiter.api.Assertions.assertNotNull;
1110
import static org.junit.jupiter.api.Assertions.assertThrows;
12-
import static org.junit.jupiter.api.Assertions.assertTrue;
1311
import static org.opensearch.sql.ast.dsl.AstDSL.agg;
1412
import static org.opensearch.sql.ast.dsl.AstDSL.aggregate;
1513
import static org.opensearch.sql.ast.dsl.AstDSL.alias;
@@ -45,7 +43,6 @@
4543
import org.opensearch.sql.ast.expression.UnresolvedArgument;
4644
import org.opensearch.sql.ast.tree.SubqueryAlias;
4745
import org.opensearch.sql.ast.tree.TableFunction;
48-
import org.opensearch.sql.ast.tree.UnresolvedPlan;
4946
import org.opensearch.sql.common.antlr.SyntaxCheckException;
5047

5148
class AstBuilderTest extends AstBuilderTestBase {
@@ -158,23 +155,94 @@ public void can_build_from_table_function() {
158155
}
159156

160157
@Test
161-
public void can_build_from_table_function_with_where_and_order() {
162-
// Verify parsing succeeds for table function with WHERE, ORDER BY, and LIMIT
163-
UnresolvedPlan plan =
158+
public void can_build_from_table_function_with_where_order_limit() {
159+
assertEquals(
160+
project(
161+
limit(
162+
sort(
163+
filter(
164+
new SubqueryAlias(
165+
"s",
166+
new TableFunction(
167+
qualifiedName("vectorSearch"),
168+
ImmutableList.of(
169+
new UnresolvedArgument("table", stringLiteral("products")),
170+
new UnresolvedArgument("field", stringLiteral("embedding")),
171+
new UnresolvedArgument(
172+
"vector", stringLiteral("[0.1,0.2]")),
173+
new UnresolvedArgument(
174+
"option", stringLiteral("k=10"))))),
175+
function("=", qualifiedName("s", "category"), stringLiteral("shoes"))),
176+
field(
177+
qualifiedName("s", "_score"),
178+
argument("asc", booleanLiteral(false)))),
179+
5,
180+
0),
181+
alias("s.title", qualifiedName("s", "title")),
182+
alias("s._score", qualifiedName("s", "_score"))),
164183
buildAST(
165184
"SELECT s.title, s._score FROM vectorSearch("
166185
+ "table='products', field='embedding', "
167186
+ "vector='[0.1,0.2]', option='k=10') AS s "
168187
+ "WHERE s.category = 'shoes' "
169188
+ "ORDER BY s._score DESC "
170-
+ "LIMIT 5");
171-
assertNotNull(plan);
172-
// Verify the plan contains the expected structure
173-
String planStr = plan.toString();
174-
assertTrue(planStr.contains("SubqueryAlias(alias=s"));
175-
assertTrue(planStr.contains("TableFunction(functionName=vectorSearch"));
176-
assertTrue(planStr.contains("UnresolvedArgument(argName=table, value=products)"));
177-
assertTrue(planStr.contains("Filter(condition==(s.category, shoes)"));
189+
+ "LIMIT 5"));
190+
}
191+
192+
@Test
193+
public void table_function_args_are_resolved_by_name_not_position() {
194+
assertEquals(
195+
project(
196+
new SubqueryAlias(
197+
"v",
198+
new TableFunction(
199+
qualifiedName("vectorSearch"),
200+
ImmutableList.of(
201+
new UnresolvedArgument("option", stringLiteral("k=10")),
202+
new UnresolvedArgument("field", stringLiteral("embedding")),
203+
new UnresolvedArgument("table", stringLiteral("products")),
204+
new UnresolvedArgument("vector", stringLiteral("[0.1,0.2]"))))),
205+
AllFields.of()),
206+
buildAST(
207+
"SELECT * FROM vectorSearch("
208+
+ "option='k=10', field='embedding', "
209+
+ "table='products', vector='[0.1,0.2]') AS v"));
210+
}
211+
212+
@Test
213+
public void table_function_arg_names_are_canonicalized() {
214+
assertEquals(
215+
project(
216+
new SubqueryAlias(
217+
"v",
218+
new TableFunction(
219+
qualifiedName("vectorSearch"),
220+
ImmutableList.of(
221+
new UnresolvedArgument("table", stringLiteral("products")),
222+
new UnresolvedArgument("field", stringLiteral("embedding")),
223+
new UnresolvedArgument("vector", stringLiteral("[0.1,0.2]")),
224+
new UnresolvedArgument("option", stringLiteral("k=10"))))),
225+
AllFields.of()),
226+
buildAST(
227+
"SELECT * FROM vectorSearch("
228+
+ "TABLE='products', FIELD='embedding', "
229+
+ "VECTOR='[0.1,0.2]', OPTION='k=10') AS v"));
230+
}
231+
232+
@Test
233+
public void table_function_allows_alias_without_as_keyword() {
234+
assertEquals(
235+
project(
236+
new SubqueryAlias(
237+
"v",
238+
new TableFunction(
239+
qualifiedName("vectorSearch"),
240+
ImmutableList.of(
241+
new UnresolvedArgument("table", stringLiteral("products")),
242+
new UnresolvedArgument("vector", stringLiteral("[0.1]"))))),
243+
AllFields.of()),
244+
buildAST(
245+
"SELECT * FROM vectorSearch(table='products', vector='[0.1]') v"));
178246
}
179247

180248
@Test

0 commit comments

Comments
 (0)