-
Notifications
You must be signed in to change notification settings - Fork 327
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
Expression update contains full multi-value info #12195
base: develop
Are you sure you want to change the base?
Conversation
|
||
if (publicTypes != null) { | ||
final Type[] allTypes = typeOfNode.findAllTypesOrNull(value, true); | ||
// Relies on the fact that allTypes appends extra types to the end of publicTypes. |
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.
Make this safer:
- assert the first
publicType.length
part is the same in both arrays - change the format of the message to avoid this array equlibristic
@@ -119,7 +129,7 @@ object Runtime { | |||
@named("expressionUpdate") | |||
case class ExpressionUpdate( | |||
expressionId: ExpressionId, | |||
expressionTypes: Option[Vector[String]], | |||
expressionType: Option[ExpressionType], |
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 (again) an incompatible change in the protocol. Shouldn't we rather start to be compatible in the absence of #7717? What about:
- keeping
expressionTypes
unchanged - adding optional
dispatchTypes
withpublicTypes = typeOfNode.findAllTypesOrNull(value, false);
- note that when
dispatchTypes
are missing, then the first type ofexpressionTypes
is dispatchable
* @param conversionType the available conversions | ||
*/ | ||
case class ExpressionType( | ||
visibleType: Vector[String], |
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 am not sure I like the terminology:
- maybe we use visible type somewhere
- but we certainly never used conversion type!
The documentation is using following terms:
- "has been cast to (the visible part)"
- "hidden types .... can be cast to."
I understand that the current terminology isn't perfect. We can change it, but consistently. We shouldn't be inventing conversionType
term in the protocol only. If we want new term, we should update (at least) docs as well.
Pull Request Description
close #12153
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.