Properly delimit identifiers in DataType expression created from Type #8845
Conversation
losipiuk
force-pushed
the
lo/delimit-type-identifiers
branch
from
8fde81a
to
d205867
Compare
losipiuk
force-pushed
the
lo/delimit-type-identifiers
branch
2 times, most recently
from
05d1413
to
f24a51d
Compare
| @@ -300,6 +301,19 @@ static DataType toDataType(TypeSignature typeSignature) | |||
| } | |||
| } | |||
|
|
|||
| private static boolean requiresDelimiting(String identifier) | |||
There was a problem hiding this comment.
This is ok for now, but technically, every name should be delimited. Once #17 is fixed, any names coming from a connector (including column types and field names in row types) will be considered as "already canonicalized", so they'll need to be surrounded in quotes when rendering as SQL to ensure proper matching semantics.
| @@ -33,7 +32,7 @@ private void assertRoundTrip(String expression) | |||
| { | |||
| assertThat(type(expression)) | |||
| .ignoringLocation() | |||
| .withComparatorForType(Comparator.comparing(identifier -> identifier.getValue().toLowerCase(Locale.ENGLISH)), Identifier.class) | |||
| .withComparatorForType(Comparator.comparing(Identifier::getCanonicalValue), Identifier.class) | |||
There was a problem hiding this comment.
This changes semantics. Is it intentional? The canonical value per SQL standard rules uses upper case. For historical reasons, and until #17 is fixed, we continue to use lower case in many places.
There was a problem hiding this comment.
Does it matter here? I assumed that for sake of testing assertion whatever normalization (upper/lower) is fine. I changed to getCanonicalValue mostly to have different matching logic for delimited (uppercasing) and non-delimited (no uppercasing) identifiers. Without the change, the test passes for field names that require delimiting even without changes to TypeSignatureTranslator. I agree this is kinda a hack though. I can take that back and assume that regression testing is solely done via testImplicitCastToRowWithFieldsRequiringDelimitation().
losipiuk
force-pushed
the
lo/delimit-type-identifiers
branch
from
f24a51d
to
2970a52
Compare
|
AC @martint . Let me know if this is good to go. |
|
CI: #8691 |
No description provided.