-
Notifications
You must be signed in to change notification settings - Fork 13k
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.
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
[FLINK-17151][table] Align Calcite's and Flink's SYMBOL types #18107
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 5cb0193 (Tue Dec 14 15:01:30 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The 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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I left a few comments/questions.
*/ | ||
@SuppressWarnings("unused") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leftover?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is used for the argument of the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then maybe only move it there on top of the ctor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or like this: public SymbolType(@SuppressWarnings("unused") Class<T> clazz) {
?
@@ -45,7 +44,8 @@ public SymbolArgumentTypeStrategy(Class<? extends Enum<? extends TableSymbol>> s | |||
public Optional<DataType> inferArgumentType( | |||
CallContext callContext, int argumentPos, boolean throwOnFailure) { | |||
final DataType argumentType = callContext.getArgumentDataTypes().get(argumentPos); | |||
if (argumentType.getLogicalType().getTypeRoot() != LogicalTypeRoot.SYMBOL) { | |||
if (argumentType.getLogicalType().getTypeRoot() != LogicalTypeRoot.SYMBOL | |||
|| !callContext.isArgumentLiteral(argumentPos)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed now? is it tested somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always needed before calling getArgumentValue
as written in the JavaDocs. A user would need to create a weird expression to have a symbol that is not a literal.
symbolClass.getSimpleName(), | ||
symbolType.getDefaultConversion().getSimpleName()); | ||
callContext | ||
.getArgumentValue(argumentPos, Enum.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused, but if L58, we don't have a symbolClass
, can we still have an Enum here?
is there a test covering this, and the "invalid" case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolArgumentTypeStrategyTest
is testing this path. We are in control of creating symbols so we can safely assume an Enum
here. Everything else is highly unlikely. For the invalid
case I would need to create an invalid expression. The alternative would have been an InvalidArgumentException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's easy to have a test like that, would be nice for completeness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that is not easy. As I said, I would need to create a completely invalid expression. Maybe some constructor checks would already prevent that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor issue, otherwise LGTM
public SymbolType(boolean isNullable) { | ||
super(isNullable, LogicalTypeRoot.SYMBOL); | ||
} | ||
|
||
public Class<T> getSymbolClass() { | ||
return symbolClass; | ||
public SymbolType() { | ||
this(true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps create 2 singletons? As I see in different places you either instantiate new SymbolType()
and new SymbolType(false)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this as a followup. We can do the same with IntType
and many other predefined types. I was also thinking about reducing the creation of objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, responded to the original comments, thx for explaining!
What is the purpose of the change
This reworks how we handle symbols throughout the stack. It removes the need for a conversion class and align the behavior with Calcite's representation of symbols. Even though we change a
@PublicEvolving
class this should have little impact for users as this type is not exposed through the API.Brief change log
SymbolType
RAW
type for symbols.Verifying this change
This change is already covered by existing tests, such as (please describe tests).
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: yesDocumentation