[FLINK-13078][table-common] Add a logical type parser #9061
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. DetailsThe Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
| * evaluation of types. | ||
| * | ||
| * <p>The enumeration is very close to the SQL standard in terms of naming and completeness. However, | ||
| * it reflects just a subset of the evolving standard and contains some extensions (such as {@code NULL}, |
There was a problem hiding this comment.
remove the opening ( or revert the closing one.
|
|
||
| private final @Nullable String database; | ||
|
|
||
| private final String typeIdentifier; |
There was a problem hiding this comment.
How about using ObjectIdentifier here?
There was a problem hiding this comment.
In the first version ObjectIdentifier allowed also partially defined paths. But we changed it to full identifiers. I would be up for relaxing this constraint to be honest as I see already a couple of String[] List<String> objects travelling through the stack.
| /** | ||
| * Parses a type string. All types will be fully resolved except for {@link UnresolvedUserDefinedType}s. | ||
| * | ||
| * <p>Throws {@link ValidationException} in case of parsing errors. |
There was a problem hiding this comment.
use @throws annotation for this.
| IDENTIFIER, | ||
|
|
||
| // e.g. "myCatalog.myDatabase." | ||
| DOT |
There was a problem hiding this comment.
nit, (also not sure if it would be better), but maybe IDENTIFIER_SEPARATOR? Similar to LIST_SEPARATOR.
| } | ||
|
|
||
| private static List<Token> tokenize(String typeString) { | ||
| final char[] chars = typeString.toCharArray(); |
There was a problem hiding this comment.
Why not just access the String directly via #charAt(). This performs unnecessary copy (but I know it is not crucial here)
|
|
||
| private LogicalType parseTimeType() { | ||
| int precision = TimeType.DEFAULT_PRECISION; | ||
| if (hasNextToken(TokenType.BEGIN_PARAMETER)) { |
There was a problem hiding this comment.
nit: use parseOptionalPrecision
|
|
||
| private LogicalType parseTimestampType() { | ||
| int precision = TimestampType.DEFAULT_PRECISION; | ||
| if (hasNextToken(TokenType.BEGIN_PARAMETER)) { |
There was a problem hiding this comment.
nit: use parseOptionalPrecision
| } | ||
| } | ||
|
|
||
| private LogicalType parseDayTimeIntervalType() { |
There was a problem hiding this comment.
How about splitting this method by the 3 cases.
There was a problem hiding this comment.
Moreover I think this method could be improved, if we first parse lower/upper resolution and then reuse DataTypes.Resolution#resolveInterval. In a pseudo code it could look sth like:
public Resolution parseAsResolution(IntervalUnit type) {
if (type == DAY) {
dayPrecision = parseOptionalPrecision(dayPrecision);
return new Resolution(type, dayPrecision);
} else if (lower == SECOND) {
fractionalPrecision = parseOptionalPrecision(fractionalPrecision);
return new Resolution(type, fractionalPrecision);
}
}
public LogicalType parseDayTimeIntervalType() {
IntervalUnit upper = tokenAsIntervalUnit();
Optional<IntervalUnit> lower = tokenAsIntervalUnit();
DataTypes.Resolution.resolveInterval(parseAsResolution(upper), lower.map(parseAsResolution).orNull)
}
There was a problem hiding this comment.
TBH I think it is not worth the effort. Usually, such classes are generated and look much worse. Unless, the standard changes we will also not need to make changes in this method. So maintainability is also not an issue. Exposing DataTypes.Resolution.resolveInterval is mixing API with internal utils.
| @Parameters(name = "{index}: [From: {0}, To: {1}]") | ||
| public static List<Object[]> testData() { | ||
| return Arrays.asList( | ||
| new Object[][]{ |
There was a problem hiding this comment.
nit: I think it would make sense to have a TestSpec object in this case, right now we have a magic null value for most of the test cases. I think it would be easier to read if it was:
TestSpec
.forString("CHAR")
.parseAsType(new CharType());
TestSpec
.forString("ROW<")
.throwsExceptionWithMessage("...")
This adds a parser for all logical types defined in FLIP-37. This closes #9061.
4 participants
What is the purpose of the change
This adds a parser for all logical types defined in FLIP-37. This parser is both useful for the SQL Client as well as type annotations for UDFs.
Brief change log
See commit messages.
Verifying this change
This change added tests and can be verified as follows:
LogicalTypeParserTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): yesDocumentation