Skip to content

Fix potential ambiguity while parsing queries involving table functions #16014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kasiafi merged 1 commit into from
Feb 9, 2023
Merged

Fix potential ambiguity while parsing queries involving table functions #16014

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1917,6 +1917,9 @@ public Node visitTableArgumentTable(SqlBaseParser.TableArgumentTableContext cont

if (context.identifier() != null) {
Identifier alias = (Identifier) visit(context.identifier());
if (context.AS() == null) {
validateArgumentAlias(alias, context.identifier());
}
List<Identifier> columnNames = null;
if (context.columnAliases() != null) {
columnNames = visit(context.columnAliases().identifier(), Identifier.class);
Expand All @@ -1934,6 +1937,9 @@ public Node visitTableArgumentQuery(SqlBaseParser.TableArgumentQueryContext cont

if (context.identifier() != null) {
Identifier alias = (Identifier) visit(context.identifier());
if (context.AS() == null) {
validateArgumentAlias(alias, context.identifier());
}
List<Identifier> columnNames = null;
if (context.columnAliases() != null) {
columnNames = visit(context.columnAliases().identifier(), Identifier.class);
Expand Down Expand Up @@ -3763,4 +3769,14 @@ private static QueryPeriod.RangeType getRangeType(Token token)
}
throw new IllegalArgumentException("Unsupported query period range type: " + token.getText());
}

private static void validateArgumentAlias(Identifier alias, ParserRuleContext context)
{
check(
alias.isDelimited() || !alias.getValue().equalsIgnoreCase("COPARTITION"),
"The word \"COPARTITION\" is ambiguous in this context. " +
"To alias an argument, precede the alias with \"AS\". " +
"To specify co-partitioning, change the argument order so that the last argument cannot be aliased.",
context);
}
}
65 changes: 65 additions & 0 deletions core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4350,6 +4350,71 @@ public void testTableFunctionTableArgumentAliasing()
ImmutableList.of())));
}

@Test
public void testCopartitionInTableArgumentAlias()
{
// table argument 'input' is aliased. The alias "copartition" is illegal in this context.
assertThatThrownBy(() -> SQL_PARSER.createStatement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
"input => TABLE(orders) copartition(a, b, c)))",
new ParsingOptions()))
.isInstanceOf(ParsingException.class)
.hasMessageMatching("line 1:54: The word \"COPARTITION\" is ambiguous in this context. " +
"To alias an argument, precede the alias with \"AS\". " +
"To specify co-partitioning, change the argument order so that the last argument cannot be aliased.");

// table argument 'input' contains an aliased relation with the alias "copartition". The alias is enclosed in the 'TABLE(...)' clause, and the argument itself is not aliased.
// The alias "copartition" is legal in this context.
assertThat(statement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
" input => TABLE(SELECT * FROM orders copartition(a, b, c))))"))
.isInstanceOf(Query.class);

// table argument 'input' is aliased. The alias "COPARTITION" is delimited, so it can cause no ambiguity with the COPARTITION clause, and is considered legal in this context.
assertThat(statement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
"input => TABLE(orders) \"COPARTITION\"(a, b, c)))"))
.isInstanceOf(Query.class);

// table argument 'input' is aliased. The alias "copartition" is preceded with the keyword "AS", so it can cause no ambiguity with the COPARTITION clause, and is considered legal in this context.
assertThat(statement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
"input => TABLE(orders) AS copartition(a, b, c)))"))
.isInstanceOf(Query.class);

// the COPARTITION word can be either the alias for argument 'input3', or part of the COPARTITION clause.
// It is parsed as the argument alias, and then fails as illegal in this context.
assertThatThrownBy(() -> SQL_PARSER.createStatement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
"input1 => TABLE(customers) PARTITION BY nationkey, " +
"input2 => TABLE(nation) PARTITION BY nationkey, " +
"input3 => TABLE(lineitem) " +
"COPARTITION(customers, nation))) ",
new ParsingOptions()))
.isInstanceOf(ParsingException.class)
.hasMessageMatching("line 1:156: The word \"COPARTITION\" is ambiguous in this context. " +
"To alias an argument, precede the alias with \"AS\". " +
"To specify co-partitioning, change the argument order so that the last argument cannot be aliased.");

// the above query does not fail if we change the order of arguments so that the last argument before the COPARTITION clause has specified partitioning.
// In such case, the COPARTITION word cannot be mistaken for alias.
// Note that this transformation of the query is always available. If the table function invocation contains the COPARTITION clause,
// at least two table arguments must have partitioning specified.
assertThat(statement(
"SELECT * " +
"FROM TABLE(some_ptf( " +
"input1 => TABLE(customers) PARTITION BY nationkey, " +
"input3 => TABLE(lineitem), " +
"input2 => TABLE(nation) PARTITION BY nationkey " +
"COPARTITION(customers, nation))) "))
.isInstanceOf(Query.class);
}

private static Query selectAllFrom(Relation relation)
{
return new Query(
Expand Down