Skip to content

Commit 32fe958

Browse files
agrawalreetikaimjalpreet
authored and
Presto CUDF CI
committed
Preserve quotedness in QualifiedName
Cherry-pick of trinodb/trino#80 Presto doesn't maintain the quotedness of an identifier when using SqlQueryFormatter so it results in throwing parsing error if quoted table name is a reserved word Co-authored-by: praveenkrishna <[email protected]> Co-authored-by: imjalpreet <[email protected]>
1 parent ea9d5f2 commit 32fe958

File tree

15 files changed

+172
-73
lines changed

15 files changed

+172
-73
lines changed

‎presto-main-base/src/main/java/com/facebook/presto/metadata/MetadataUtil.java‎

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.facebook.presto.spi.security.PrestoPrincipal;
3030
import com.facebook.presto.sql.analyzer.SemanticException;
3131
import com.facebook.presto.sql.tree.GrantorSpecification;
32+
import com.facebook.presto.sql.tree.Identifier;
3233
import com.facebook.presto.sql.tree.Node;
3334
import com.facebook.presto.sql.tree.PrincipalSpecification;
3435
import com.facebook.presto.sql.tree.QualifiedName;
@@ -127,17 +128,17 @@ public static CatalogSchemaName createCatalogSchemaName(Session session, Node no
127128
String schemaName = session.getSchema().orElse(null);
128129

129130
if (schema.isPresent()) {
130-
List<String> parts = schema.get().getOriginalParts();
131+
List<Identifier> parts = schema.get().getOriginalParts();
131132
if (parts.size() > 2) {
132133
throw new SemanticException(INVALID_SCHEMA_NAME, node, "Too many parts in schema name: %s", schema.get());
133134
}
134135
if (parts.size() == 2) {
135-
catalogName = parts.get(0);
136+
catalogName = parts.get(0).getValue();
136137
}
137138
if (catalogName == null) {
138139
throw new SemanticException(CATALOG_NOT_SPECIFIED, node, "Catalog must be specified when session catalog is not set");
139140
}
140-
schemaName = metadata.normalizeIdentifier(session, catalogName, schema.get().getOriginalSuffix());
141+
schemaName = metadata.normalizeIdentifier(session, catalogName, schema.get().getOriginalSuffix().getValue());
141142
}
142143

143144
if (catalogName == null) {
@@ -158,11 +159,11 @@ public static QualifiedObjectName createQualifiedObjectName(Session session, Nod
158159
throw new PrestoException(SYNTAX_ERROR, format("Too many dots in table name: %s", name));
159160
}
160161

161-
List<String> parts = Lists.reverse(name.getOriginalParts());
162-
String objectName = parts.get(0);
163-
String schemaName = (parts.size() > 1) ? parts.get(1) : session.getSchema().orElseThrow(() ->
162+
List<Identifier> parts = Lists.reverse(name.getOriginalParts());
163+
String objectName = parts.get(0).getValue();
164+
String schemaName = (parts.size() > 1) ? parts.get(1).getValue() : session.getSchema().orElseThrow(() ->
164165
new SemanticException(SCHEMA_NOT_SPECIFIED, node, "Schema must be specified when session schema is not set"));
165-
String catalogName = (parts.size() > 2) ? parts.get(2) : session.getCatalog().orElseThrow(() ->
166+
String catalogName = (parts.size() > 2) ? parts.get(2).getValue() : session.getCatalog().orElseThrow(() ->
166167
new SemanticException(CATALOG_NOT_SPECIFIED, node, "Catalog must be specified when session catalog is not set"));
167168

168169
catalogName = catalogName.toLowerCase(ENGLISH);
@@ -172,11 +173,6 @@ public static QualifiedObjectName createQualifiedObjectName(Session session, Nod
172173
return new QualifiedObjectName(catalogName, schemaName, objectName);
173174
}
174175

175-
public static QualifiedName createQualifiedName(QualifiedObjectName name)
176-
{
177-
return QualifiedName.of(name.getCatalogName(), name.getSchemaName(), name.getObjectName());
178-
}
179-
180176
public static Optional<CatalogMetadata> getOptionalCatalogMetadata(Session session, TransactionManager transactionManager, String catalogName)
181177
{
182178
return transactionManager.getOptionalCatalogMetadata(session.getRequiredTransactionId(), catalogName);

‎presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/ExpressionAnalyzer.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,7 +470,7 @@ protected Type visitSymbolReference(SymbolReference node, StackableAstVisitorCon
470470
@Override
471471
protected Type visitIdentifier(Identifier node, StackableAstVisitorContext<Context> context)
472472
{
473-
QualifiedName name = QualifiedName.of(node.getValue());
473+
QualifiedName name = QualifiedName.of(ImmutableList.of(new Identifier(node.getValue(), node.isDelimited())));
474474
Optional<ResolvedField> resolvedField = context.getContext().getScope().tryResolveField(node, name);
475475
if (!resolvedField.isPresent() && outerScopeSymbolTypes.containsKey(NodeRef.of(node))) {
476476
return setExpressionType(node, outerScopeSymbolTypes.get(NodeRef.of(node)));
@@ -1513,7 +1513,8 @@ protected Type visitLambdaExpression(LambdaExpression node, StackableAstVisitorC
15131513
fieldToLambdaArgumentDeclaration.putAll(context.getContext().getFieldToLambdaArgumentDeclaration());
15141514
}
15151515
for (LambdaArgumentDeclaration lambdaArgument : lambdaArguments) {
1516-
ResolvedField resolvedField = lambdaScope.resolveField(lambdaArgument, QualifiedName.of(lambdaArgument.getName().getValue()));
1516+
QualifiedName name = QualifiedName.of(ImmutableList.of(new Identifier(lambdaArgument.getName().getValue(), lambdaArgument.getName().isDelimited())));
1517+
ResolvedField resolvedField = lambdaScope.resolveField(lambdaArgument, name);
15171518
fieldToLambdaArgumentDeclaration.put(FieldId.from(resolvedField), lambdaArgument);
15181519
}
15191520

‎presto-main-base/src/main/java/com/facebook/presto/sql/analyzer/StatementAnalyzer.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,6 @@
310310
import static com.google.common.collect.ImmutableList.toImmutableList;
311311
import static com.google.common.collect.ImmutableMap.toImmutableMap;
312312
import static com.google.common.collect.ImmutableSet.toImmutableSet;
313-
import static com.google.common.collect.Iterables.getLast;
314313
import static java.lang.Math.toIntExact;
315314
import static java.lang.String.format;
316315
import static java.util.Collections.emptyList;
@@ -2764,7 +2763,7 @@ else if (expression instanceof DereferenceExpression) {
27642763

27652764
if (!field.isPresent()) {
27662765
if (name != null) {
2767-
field = Optional.of(new Identifier(getLast(name.getOriginalParts())));
2766+
field = Optional.of(name.getOriginalSuffix());
27682767
}
27692768
}
27702769

‎presto-main-base/src/main/java/com/facebook/presto/sql/rewrite/ShowQueriesRewrite.java‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,6 @@
114114
import static com.facebook.presto.metadata.MetadataListing.listCatalogs;
115115
import static com.facebook.presto.metadata.MetadataListing.listSchemas;
116116
import static com.facebook.presto.metadata.MetadataUtil.createCatalogSchemaName;
117-
import static com.facebook.presto.metadata.MetadataUtil.createQualifiedName;
118117
import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName;
119118
import static com.facebook.presto.metadata.MetadataUtil.getConnectorIdOrThrow;
120119
import static com.facebook.presto.metadata.SessionFunctionHandle.SESSION_NAMESPACE;
@@ -163,6 +162,7 @@
163162
import static com.google.common.base.Strings.nullToEmpty;
164163
import static com.google.common.collect.ImmutableList.toImmutableList;
165164
import static com.google.common.collect.ImmutableSet.toImmutableSet;
165+
import static com.google.common.collect.Iterables.getLast;
166166
import static java.lang.String.format;
167167
import static java.util.Locale.ENGLISH;
168168
import static java.util.Objects.requireNonNull;
@@ -505,7 +505,7 @@ protected Node visitShowCreate(ShowCreate node, Void context)
505505
accessControl.checkCanShowCreateTable(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), objectName);
506506

507507
CreateView.Security security = (viewDefinition.get().isRunAsInvoker()) ? CreateView.Security.INVOKER : CreateView.Security.DEFINER;
508-
String sql = formatSql(new CreateView(createQualifiedName(objectName), query, false, Optional.of(security)), Optional.of(parameters)).trim();
508+
String sql = formatSql(new CreateView(getQualifiedName(node, objectName), query, false, Optional.of(security)), Optional.of(parameters)).trim();
509509
return singleValueQuery("Create View", sql);
510510
}
511511

@@ -533,7 +533,7 @@ protected Node visitShowCreate(ShowCreate node, Void context)
533533

534534
CreateMaterializedView createMaterializedView = new CreateMaterializedView(
535535
Optional.empty(),
536-
createQualifiedName(objectName),
536+
getQualifiedName(node, objectName),
537537
query,
538538
false,
539539
propertyNodes,
@@ -689,6 +689,16 @@ protected Node visitShowCreateFunction(ShowCreateFunction node, Void context)
689689
ordering(ascending("argument_types")));
690690
}
691691

692+
private QualifiedName getQualifiedName(ShowCreate node, QualifiedObjectName objectName)
693+
{
694+
List<Identifier> parts = node.getName().getOriginalParts();
695+
Identifier tableName = getLast(parts);
696+
Identifier schemaName = parts.size() > 1 ? parts.get(parts.size() - 2) : new Identifier(objectName.getSchemaName());
697+
Identifier catalogName = (parts.size() > 2) ? parts.get(0) : new Identifier(objectName.getCatalogName());
698+
699+
return QualifiedName.of(ImmutableList.of(catalogName, schemaName, tableName));
700+
}
701+
692702
private List<Property> buildProperties(
693703
Object objectName,
694704
StandardErrorCode errorCode,

‎presto-main-base/src/test/java/com/facebook/presto/sql/TestSqlFormatter.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package com.facebook.presto.sql;
15+
16+
import com.facebook.presto.sql.parser.ParsingOptions;
17+
import com.facebook.presto.sql.parser.SqlParser;
18+
import com.facebook.presto.sql.tree.Statement;
19+
import org.testng.annotations.Test;
20+
21+
import java.util.Optional;
22+
23+
import static com.facebook.presto.sql.SqlFormatterUtil.getFormattedSql;
24+
import static java.lang.String.format;
25+
import static org.testng.Assert.assertEquals;
26+
27+
public class TestSqlFormatter
28+
{
29+
@Test
30+
public void testSimpleExpression()
31+
{
32+
assertQuery("SELECT id\nFROM\n public.orders\n");
33+
assertQuery("SELECT id\nFROM\n \"public\".\"order\"\n");
34+
assertQuery("SELECT id\nFROM\n \"public\".\"order\"\"2\"\n");
35+
}
36+
37+
@Test
38+
public void testQuotedColumnNames()
39+
{
40+
assertQuery("SELECT \"Id\"\nFROM\n public.orders\n");
41+
assertQuery("SELECT \"order\".\"Name\"\nFROM\n \"public\".\"orders\"\n");
42+
assertQuery("SELECT \"a\".\"b\".\"C\"\nFROM\n \"schema\".\"table\"\n");
43+
assertQuery("ALTER TABLE sales.orders RENAME COLUMN \"OrderId\" TO \"OrderID_New\"");
44+
assertQuery("ALTER TABLE sales.orders ADD COLUMN \"Customer\" VARCHAR");
45+
assertQuery("ALTER TABLE sales.orders DROP COLUMN \"Customer\"");
46+
}
47+
48+
private void assertQuery(String query)
49+
{
50+
SqlParser parser = new SqlParser();
51+
Statement statement = parser.createStatement(query, new ParsingOptions());
52+
String formattedQuery = getFormattedSql(statement, parser, Optional.empty());
53+
assertEquals(formattedQuery, query);
54+
assertEquals(formattedQuery, query, format("Formatted SQL did not match original for query: %s", query));
55+
}
56+
}

‎presto-parser/src/main/java/com/facebook/presto/sql/SqlFormatter.java‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1156,12 +1156,10 @@ private String formatRoutineCharacteristicName(Enum characteristic)
11561156
return characteristic.name().replace("_", " ");
11571157
}
11581158

1159-
private static String formatName(String name)
1159+
private static String formatName(Identifier name)
11601160
{
1161-
if (NAME_PATTERN.matcher(name).matches()) {
1162-
return name;
1163-
}
1164-
return "\"" + name.replace("\"", "\"\"") + "\"";
1161+
String delimiter = name.isDelimited() ? "\"" : "";
1162+
return delimiter + name.getValue().replace("\"", "\"\"") + delimiter;
11651163
}
11661164

11671165
private static String formatName(QualifiedName name)

‎presto-parser/src/main/java/com/facebook/presto/sql/parser/AstBuilder.java‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2320,11 +2320,7 @@ private static LikeClause.PropertiesOption getPropertiesOption(Token token)
23202320

23212321
private QualifiedName getQualifiedName(SqlBaseParser.QualifiedNameContext context)
23222322
{
2323-
List<String> parts = visit(context.identifier(), Identifier.class).stream()
2324-
.map(Identifier::getValue) // TODO: preserve quotedness
2325-
.collect(Collectors.toList());
2326-
2327-
return QualifiedName.of(parts);
2323+
return QualifiedName.of(visit(context.identifier(), Identifier.class));
23282324
}
23292325

23302326
private static boolean isDistinct(SqlBaseParser.SetQuantifierContext setQuantifier)

‎presto-parser/src/main/java/com/facebook/presto/sql/tree/DereferenceExpression.java‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515

1616
import com.google.common.collect.ImmutableList;
1717

18-
import java.util.ArrayList;
1918
import java.util.List;
2019
import java.util.Objects;
2120
import java.util.Optional;
@@ -75,7 +74,20 @@ public Identifier getField()
7574
*/
7675
public static QualifiedName getQualifiedName(DereferenceExpression expression)
7776
{
78-
List<String> parts = tryParseParts(expression.base, expression.field.getValue());
77+
List<Identifier> parts = null;
78+
if (expression.base instanceof Identifier) {
79+
parts = ImmutableList.of((Identifier) expression.base, expression.field);
80+
}
81+
else if (expression.base instanceof DereferenceExpression) {
82+
QualifiedName baseQualifiedName = getQualifiedName((DereferenceExpression) expression.base);
83+
if (baseQualifiedName != null) {
84+
ImmutableList.Builder<Identifier> builder = ImmutableList.builder();
85+
builder.addAll(baseQualifiedName.getOriginalParts());
86+
builder.add(expression.field);
87+
parts = builder.build();
88+
}
89+
}
90+
7991
return parts == null ? null : QualifiedName.of(parts);
8092
}
8193

@@ -95,22 +107,6 @@ public static Expression from(QualifiedName name)
95107
return result;
96108
}
97109

98-
private static List<String> tryParseParts(Expression base, String fieldName)
99-
{
100-
if (base instanceof Identifier) {
101-
return ImmutableList.of(((Identifier) base).getValue(), fieldName);
102-
}
103-
else if (base instanceof DereferenceExpression) {
104-
QualifiedName baseQualifiedName = getQualifiedName((DereferenceExpression) base);
105-
if (baseQualifiedName != null) {
106-
List<String> newList = new ArrayList<>(baseQualifiedName.getOriginalParts());
107-
newList.add(fieldName);
108-
return newList;
109-
}
110-
}
111-
return null;
112-
}
113-
114110
@Override
115111
public boolean equals(Object o)
116112
{

‎presto-parser/src/main/java/com/facebook/presto/sql/tree/QualifiedName.java‎

Lines changed: 13 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,52 +19,52 @@
1919

2020
import java.util.List;
2121
import java.util.Optional;
22+
import java.util.stream.Collectors;
2223

2324
import static com.google.common.base.Preconditions.checkArgument;
25+
import static com.google.common.collect.ImmutableList.toImmutableList;
2426
import static com.google.common.collect.Iterables.getLast;
2527
import static com.google.common.collect.Iterables.isEmpty;
26-
import static com.google.common.collect.Iterables.transform;
2728
import static java.util.Locale.ENGLISH;
2829
import static java.util.Objects.requireNonNull;
2930

3031
public class QualifiedName
3132
{
3233
private final List<String> parts;
33-
private final List<String> originalParts;
34+
private final List<Identifier> originalParts;
3435

3536
public static QualifiedName of(String first, String... rest)
3637
{
3738
requireNonNull(first, "first is null");
38-
return of(ImmutableList.copyOf(Lists.asList(first, rest)));
39+
return of(ImmutableList.copyOf(Lists.asList(first, rest).stream().map(Identifier::new).collect(Collectors.toList())));
3940
}
4041

4142
public static QualifiedName of(String name)
4243
{
4344
requireNonNull(name, "name is null");
44-
return of(ImmutableList.of(name));
45+
return of(ImmutableList.of(new Identifier(name)));
4546
}
4647

47-
public static QualifiedName of(Iterable<String> originalParts)
48+
public static QualifiedName of(Iterable<Identifier> originalParts)
4849
{
4950
requireNonNull(originalParts, "originalParts is null");
5051
checkArgument(!isEmpty(originalParts), "originalParts is empty");
51-
List<String> parts = ImmutableList.copyOf(transform(originalParts, part -> part.toLowerCase(ENGLISH)));
5252

53-
return new QualifiedName(ImmutableList.copyOf(originalParts), parts);
53+
return new QualifiedName(ImmutableList.copyOf(originalParts));
5454
}
5555

56-
private QualifiedName(List<String> originalParts, List<String> parts)
56+
private QualifiedName(List<Identifier> originalParts)
5757
{
5858
this.originalParts = originalParts;
59-
this.parts = parts;
59+
this.parts = originalParts.stream().map(identifier -> identifier.getValue().toLowerCase(ENGLISH)).collect(toImmutableList());
6060
}
6161

6262
public List<String> getParts()
6363
{
6464
return parts;
6565
}
6666

67-
public List<String> getOriginalParts()
67+
public List<Identifier> getOriginalParts()
6868
{
6969
return originalParts;
7070
}
@@ -85,9 +85,8 @@ public Optional<QualifiedName> getPrefix()
8585
return Optional.empty();
8686
}
8787

88-
List<String> originalSubList = originalParts.subList(0, originalParts.size() - 1);
89-
List<String> subList = parts.subList(0, parts.size() - 1);
90-
return Optional.of(new QualifiedName(originalSubList, subList));
88+
List<Identifier> subList = originalParts.subList(0, originalParts.size() - 1);
89+
return Optional.of(new QualifiedName(subList));
9190
}
9291

9392
public boolean hasSuffix(QualifiedName suffix)
@@ -106,7 +105,7 @@ public String getSuffix()
106105
return getLast(parts);
107106
}
108107

109-
public String getOriginalSuffix()
108+
public Identifier getOriginalSuffix()
110109
{
111110
return getLast(originalParts);
112111
}

0 commit comments

Comments
 (0)