Skip to content

Commit b62d08d

Browse files
alecgrieseralecgrieser
authored
Revert Identifier Translation for Protobuf Compliant Names (#3726)
This reverts #3696 and #3706, which together were trying to handle translating the SQL names to protobuf identifiers. This had some issues, especially when it came to function and view name handling, and so we want to redo this in a slightly different way. While we work on that fix, this backs out the change.
1 parent 345de8d commit b62d08d

File tree

12 files changed

+51
-710
lines changed

12 files changed

+51
-710
lines changed

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/ddl/RecordLayerCatalogQueryFactory.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@
3737
import com.apple.foundationdb.relational.api.metadata.SchemaTemplate;
3838
import com.apple.foundationdb.relational.recordlayer.ArrayRow;
3939
import com.apple.foundationdb.relational.recordlayer.IteratorResultSet;
40-
import com.apple.foundationdb.relational.recordlayer.metadata.DataTypeUtils;
4140

4241
import javax.annotation.Nonnull;
4342
import java.net.URI;
@@ -65,7 +64,7 @@ public Type getResultSetMetadata() {
6564
public RelationalResultSet executeAction(Transaction txn) throws RelationalException {
6665
final Schema schema = catalog.loadSchema(txn, dbId, schemaId);
6766

68-
final List<String> tableNames = schema.getTables().stream().map(Metadata::getName).map(DataTypeUtils::toUserIdentifier)
67+
final List<String> tableNames = schema.getTables().stream().map(Metadata::getName)
6968
.collect(Collectors.toList());
7069

7170
final List<String> indexNames = schema.getTables().stream().flatMap(t -> t.getIndexes().stream()).map(Metadata::getName)

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/metadata/DataTypeUtils.java‎

Lines changed: 9 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,13 @@
2121
package com.apple.foundationdb.relational.recordlayer.metadata;
2222

2323
import com.apple.foundationdb.annotation.API;
24+
2425
import com.apple.foundationdb.record.query.plan.cascades.typing.Type;
2526
import com.apple.foundationdb.relational.api.exceptions.ErrorCode;
2627
import com.apple.foundationdb.relational.api.metadata.DataType;
2728
import com.apple.foundationdb.relational.util.Assert;
2829
import com.apple.foundationdb.relational.util.SpotBugsSuppressWarnings;
30+
2931
import com.google.common.collect.BiMap;
3032
import com.google.common.collect.HashBiMap;
3133

@@ -34,20 +36,11 @@
3436
import java.util.Locale;
3537
import java.util.Optional;
3638
import java.util.UUID;
37-
import java.util.regex.Pattern;
3839
import java.util.stream.Collectors;
3940

4041
@API(API.Status.EXPERIMENTAL)
4142
public class DataTypeUtils {
4243

43-
private static final String DOUBLE_UNDERSCORE_ESCAPE = "__0";
44-
private static final String DOLLAR_ESCAPE = "__1";
45-
private static final String DOT_ESCAPE = "__2";
46-
47-
private static final List<String> INVALID_START_SEQUENCES = List.of(".", "$", DOUBLE_UNDERSCORE_ESCAPE, DOLLAR_ESCAPE, DOT_ESCAPE);
48-
49-
private static final Pattern VALID_PROTOBUF_COMPLIANT_NAME_PATTERN = Pattern.compile("^[A-Za-z_][A-Za-z0-9_]*$");
50-
5144
@Nonnull
5245
private static final BiMap<DataType, Type> primitivesMap;
5346

@@ -62,20 +55,6 @@ public class DataTypeUtils {
6255
@SpotBugsSuppressWarnings(value = "NP_NONNULL_RETURN_VIOLATION", justification = "should never happen, there is failUnchecked directly before that.")
6356
@Nonnull
6457
public static DataType toRelationalType(@Nonnull final Type type) {
65-
return toRelationalType(type, false);
66-
}
67-
68-
/**
69-
* Converts a Record Layer {@link Type} into a Relational {@link DataType}.
70-
*
71-
* Note: This method is expensive, use with care, i.e. try to cache its result as much as possible.
72-
*
73-
* @param type The Relational data type.
74-
* @return The corresponding Record Layer type.
75-
*/
76-
@SpotBugsSuppressWarnings(value = "NP_NONNULL_RETURN_VIOLATION", justification = "should never happen, there is failUnchecked directly before that.")
77-
@Nonnull
78-
public static DataType toRelationalType(@Nonnull final Type type, boolean toUserIdentifier) {
7958
if (primitivesMap.containsValue(type)) {
8059
return primitivesMap.inverse().get(type);
8160
}
@@ -96,63 +75,32 @@ public static DataType toRelationalType(@Nonnull final Type type, boolean toUser
9675
switch (typeCode) {
9776
case RECORD:
9877
final var record = (Type.Record) type;
99-
final var columns = record.getFields().stream().map(field -> {
100-
final var fieldName = toUserIdentifier ? DataTypeUtils.toUserIdentifier(field.getFieldName()) : field.getFieldName();
101-
return DataType.StructType.Field.from(fieldName, toRelationalType(field.getFieldType(), toUserIdentifier), field.getFieldIndex());
102-
}).collect(Collectors.toList());
103-
final var name = record.getName() == null ? getUniqueName() : (toUserIdentifier ? DataTypeUtils.toUserIdentifier(record.getName()) : record.getName());
104-
return DataType.StructType.from(name, columns, record.isNullable());
78+
final var columns = record.getFields().stream().map(field -> DataType.StructType.Field.from(field.getFieldName(), toRelationalType(field.getFieldType()), field.getFieldIndex())).collect(Collectors.toList());
79+
return DataType.StructType.from(record.getName() == null ? toProtoBufCompliantName(UUID.randomUUID().toString()) : record.getName(), columns, record.isNullable());
10580
case ARRAY:
10681
final var asArray = (Type.Array) type;
10782
return DataType.ArrayType.from(toRelationalType(Assert.notNullUnchecked(asArray.getElementType())), asArray.isNullable());
10883
case ENUM:
10984
final var asEnum = (Type.Enum) type;
11085
final var enumValues = asEnum.getEnumValues().stream().map(v -> DataType.EnumType.EnumValue.of(v.getName(), v.getNumber())).collect(Collectors.toList());
111-
return DataType.EnumType.from(asEnum.getName() == null ? getUniqueName() : asEnum.getName(), enumValues, asEnum.isNullable());
86+
return DataType.EnumType.from(asEnum.getName() == null ? toProtoBufCompliantName(UUID.randomUUID().toString()) : asEnum.getName(), enumValues, asEnum.isNullable());
11287
default:
11388
Assert.failUnchecked(String.format(Locale.ROOT, "unexpected type %s", type));
11489
return null; // make compiler happy.
11590
}
11691
}
11792

11893
@Nonnull
119-
private static String getUniqueName() {
120-
final var uuid = UUID.randomUUID().toString();
121-
final var modified = uuid.replace("-", "_");
122-
final char c = uuid.charAt(0);
94+
private static String toProtoBufCompliantName(@Nonnull final String input) {
95+
Assert.thatUnchecked(input.length() > 0);
96+
final var modified = input.replace("-", "_");
97+
final char c = input.charAt(0);
12398
if (c == '_' || ('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z')) {
12499
return modified;
125100
}
126101
return "id" + modified;
127102
}
128103

129-
@Nonnull
130-
public static String toProtoBufCompliantName(final String name) {
131-
Assert.thatUnchecked(INVALID_START_SEQUENCES.stream().noneMatch(name::startsWith), ErrorCode.INVALID_NAME, "name cannot start with %s", INVALID_START_SEQUENCES);
132-
String translated;
133-
if (name.startsWith("__")) {
134-
translated = "__" + translateSpecialCharacters(name.substring(2));
135-
} else {
136-
Assert.thatUnchecked(!name.isEmpty(), ErrorCode.INVALID_NAME, "name cannot be empty String.");
137-
translated = translateSpecialCharacters(name);
138-
}
139-
checkValidProtoBufCompliantName(translated);
140-
return translated;
141-
}
142-
143-
@Nonnull
144-
private static String translateSpecialCharacters(final String userIdentifier) {
145-
return userIdentifier.replace("__", DOUBLE_UNDERSCORE_ESCAPE).replace("$", DOLLAR_ESCAPE).replace(".", DOT_ESCAPE);
146-
}
147-
148-
public static void checkValidProtoBufCompliantName(String name) {
149-
Assert.thatUnchecked(VALID_PROTOBUF_COMPLIANT_NAME_PATTERN.matcher(name).matches(), ErrorCode.INVALID_NAME, name + " is not a valid name!");
150-
}
151-
152-
public static String toUserIdentifier(String protoIdentifier) {
153-
return protoIdentifier.replace(DOT_ESCAPE, ".").replace(DOLLAR_ESCAPE, "$").replace(DOUBLE_UNDERSCORE_ESCAPE, "__");
154-
}
155-
156104
/**
157105
* Converts a given Relational {@link DataType} into a corresponding Record Layer {@link Type}.
158106
*

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/Identifier.java‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,15 +22,13 @@
2222

2323
import com.apple.foundationdb.annotation.API;
2424

25-
import com.apple.foundationdb.relational.recordlayer.metadata.DataTypeUtils;
2625
import com.google.common.collect.ImmutableList;
2726

2827
import javax.annotation.Nonnull;
2928
import java.util.Collection;
3029
import java.util.List;
3130
import java.util.Objects;
3231
import java.util.function.Function;
33-
import java.util.stream.Collectors;
3432

3533
@API(API.Status.EXPERIMENTAL)
3634
public class Identifier {
@@ -144,12 +142,6 @@ public boolean qualifiedWith(@Nonnull Identifier identifier) {
144142
return true;
145143
}
146144

147-
@Nonnull
148-
public static Identifier toProtobufCompliant(@Nonnull final Identifier identifier) {
149-
final var qualifier = identifier.getQualifier().stream().map(DataTypeUtils::toProtoBufCompliantName).collect(Collectors.toList());
150-
return Identifier.of(DataTypeUtils.toProtoBufCompliantName(identifier.getName()), qualifier);
151-
}
152-
153145
@Override
154146
public boolean equals(Object obj) {
155147
if (obj == this) {

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/QueryPlan.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ private RelationalResultSet executePhysicalPlan(@Nonnull final RecordLayerSchema
404404
parsedContinuation.getExecutionState(),
405405
executeProperties));
406406
final var currentPlanHashMode = OptionsUtils.getCurrentPlanHashMode(options);
407-
final var dataType = (DataType.StructType) DataTypeUtils.toRelationalType(type, true);
407+
final var dataType = (DataType.StructType) DataTypeUtils.toRelationalType(type);
408408
return executionContext.metricCollector.clock(RelationalMetric.RelationalEvent.CREATE_RESULT_SET_ITERATOR, () -> {
409409
final ResumableIterator<Row> iterator = RecordLayerIterator.create(cursor, messageFDBQueriedRecord -> new MessageTuple(messageFDBQueriedRecord.getMessage()));
410410
return new RecordLayerResultSet(RelationalStructMetaData.of(dataType), iterator, connection,

‎fdb-relational-core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/visitors/DdlVisitor.java‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ public DataType visitFunctionColumnType(@Nonnull final RelationalParser.Function
107107
final var semanticAnalyzer = getDelegate().getSemanticAnalyzer();
108108
final SemanticAnalyzer.ParsedTypeInfo typeInfo;
109109
if (ctx.customType != null) {
110-
final var columnType = Identifier.toProtobufCompliant(visitUid(ctx.customType));
110+
final var columnType = visitUid(ctx.customType);
111111
typeInfo = SemanticAnalyzer.ParsedTypeInfo.ofCustomType(columnType, true, false);
112112
} else {
113113
typeInfo = SemanticAnalyzer.ParsedTypeInfo.ofPrimitiveType(ctx.primitiveType(), true, false);
@@ -141,7 +141,7 @@ public DataType visitColumnType(@Nonnull RelationalParser.ColumnTypeContext ctx)
141141
@Nonnull
142142
@Override
143143
public RecordLayerColumn visitColumnDefinition(@Nonnull RelationalParser.ColumnDefinitionContext ctx) {
144-
final var columnId = Identifier.toProtobufCompliant(visitUid(ctx.colName));
144+
final var columnId = visitUid(ctx.colName);
145145
final var isRepeated = ctx.ARRAY() != null;
146146
final var isNullable = ctx.columnConstraint() != null ? (Boolean) ctx.columnConstraint().accept(this) : true;
147147
// TODO: We currently do not support NOT NULL for any type other than ARRAY. This is because there is no way to
@@ -153,7 +153,7 @@ public RecordLayerColumn visitColumnDefinition(@Nonnull RelationalParser.ColumnD
153153
final var semanticAnalyzer = getDelegate().getSemanticAnalyzer();
154154
final SemanticAnalyzer.ParsedTypeInfo typeInfo;
155155
if (ctx.columnType().customType != null) {
156-
final var columnType = Identifier.toProtobufCompliant(visitUid(ctx.columnType().customType));
156+
final var columnType = visitUid(ctx.columnType().customType);
157157
typeInfo = SemanticAnalyzer.ParsedTypeInfo.ofCustomType(columnType, isNullable, isRepeated);
158158
} else {
159159
typeInfo = SemanticAnalyzer.ParsedTypeInfo.ofPrimitiveType(ctx.columnType().primitiveType(), isNullable, isRepeated);
@@ -165,15 +165,14 @@ public RecordLayerColumn visitColumnDefinition(@Nonnull RelationalParser.ColumnD
165165
@Nonnull
166166
@Override
167167
public RecordLayerTable visitTableDefinition(@Nonnull RelationalParser.TableDefinitionContext ctx) {
168-
final var tableId = Identifier.toProtobufCompliant(visitUid(ctx.uid()));
168+
final var tableId = visitUid(ctx.uid());
169169
final var columns = ctx.columnDefinition().stream().map(this::visitColumnDefinition).collect(ImmutableList.toImmutableList());
170170
final var tableBuilder = RecordLayerTable.newBuilder(metadataBuilder.isIntermingleTables())
171171
.setName(tableId.getName())
172172
.addColumns(columns);
173173
if (ctx.primaryKeyDefinition().fullIdList() != null) {
174174
visitFullIdList(ctx.primaryKeyDefinition().fullIdList())
175175
.stream()
176-
.map(Identifier::toProtobufCompliant)
177176
.map(Identifier::fullyQualifiedName)
178177
.forEach(tableBuilder::addPrimaryKeyPart);
179178
}
@@ -183,7 +182,7 @@ public RecordLayerTable visitTableDefinition(@Nonnull RelationalParser.TableDefi
183182
@Nonnull
184183
@Override
185184
public RecordLayerTable visitStructDefinition(@Nonnull RelationalParser.StructDefinitionContext ctx) {
186-
final var structId = Identifier.toProtobufCompliant(visitUid(ctx.uid()));
185+
final var structId = visitUid(ctx.uid());
187186
final var columns = ctx.columnDefinition().stream().map(this::visitColumnDefinition).collect(ImmutableList.toImmutableList());
188187
final var structBuilder = RecordLayerTable.newBuilder(metadataBuilder.isIntermingleTables())
189188
.setName(structId.getName())
@@ -341,7 +340,7 @@ private RecordLayerInvokedRoutine getInvokedRoutineMetadata(@Nonnull final Parse
341340
final var isTemporary = functionCtx instanceof RelationalParser.CreateTempFunctionContext;
342341

343342
// 1. get the function name.
344-
final var functionName = Identifier.toProtobufCompliant(visitFullId(functionSpecCtx.schemaQualifiedRoutineName)).toString();
343+
final var functionName = visitFullId(functionSpecCtx.schemaQualifiedRoutineName).toString();
345344

346345
// 2. get the function SQL definition string.
347346
final var queryString = getDelegate().getPlanGenerationContext().getQuery();
@@ -379,7 +378,7 @@ private RecordLayerView getViewMetadata(@Nonnull final RelationalParser.ViewDefi
379378
getDelegate().replaceSchemaTemplate(ddlCatalog);
380379

381380
// 1. get the view name.
382-
final var viewName = Identifier.toProtobufCompliant(visitFullId(viewCtx.viewName)).getName();
381+
final var viewName = visitFullId(viewCtx.viewName).toString();
383382

384383
// 2. get the view SQL definition string.
385384
final var queryString = getDelegate().getPlanGenerationContext().getQuery();
@@ -413,7 +412,7 @@ public ProceduralPlan visitCreateTempFunction(@Nonnull RelationalParser.CreateTe
413412

414413
@Override
415414
public ProceduralPlan visitDropTempFunction(@Nonnull RelationalParser.DropTempFunctionContext ctx) {
416-
final var functionName = Identifier.toProtobufCompliant(visitFullId(ctx.schemaQualifiedRoutineName)).toString();
415+
final var functionName = visitFullId(ctx.schemaQualifiedRoutineName).toString();
417416
var throwIfNotExists = ctx.IF() == null && ctx.EXISTS() == null;
418417
return ProceduralPlan.of(metadataOperationsFactory.getDropTemporaryFunctionConstantAction(throwIfNotExists, functionName));
419418
}
@@ -432,7 +431,7 @@ private UserDefinedFunction visitSqlInvokedFunction(@Nonnull final RelationalPar
432431
@Nonnull final RelationalParser.RoutineBodyContext bodyCtx,
433432
boolean isTemporary) {
434433
// get the function name.
435-
final var functionName = Identifier.toProtobufCompliant(visitFullId(functionSpecCtx.schemaQualifiedRoutineName)).toString();
434+
final var functionName = visitFullId(functionSpecCtx.schemaQualifiedRoutineName).toString();
436435

437436
// run implementation-specific validations.
438437
final var props = functionSpecCtx.routineCharacteristics();
@@ -540,7 +539,7 @@ public Expressions visitSqlParameterDeclarations(final RelationalParser.SqlParam
540539
@Override
541540
public Expression visitSqlParameterDeclaration(@Nonnull RelationalParser.SqlParameterDeclarationContext ctx) {
542541
Assert.thatUnchecked(ctx.sqlParameterName != null, "unnamed parameters not supported");
543-
final var parameterName = Identifier.toProtobufCompliant(visitUid(ctx.sqlParameterName));
542+
final var parameterName = visitUid(ctx.sqlParameterName);
544543
final var parameterType = visitFunctionColumnType(ctx.parameterType);
545544
final var underlyingType = DataTypeUtils.toRecordLayerType(parameterType);
546545
Assert.thatUnchecked(parameterType.isResolved());

0 commit comments

Comments
 (0)