Change java type of UNKNOWN from void to boolean#11116
Change java type of UNKNOWN from void to boolean#11116jessesleeping merged 1 commit intoprestodb:masterfrom
Conversation
|
The failed test seems unrelated. I passed it locally on my laptop. |
haozhun
left a comment
haozhun
left a comment
There was a problem hiding this comment.
Some minor inline comments.
In addition, in the commit message, also put in an explanation on why we choose boolean.
There was a problem hiding this comment.
throw new IllegalArgumentException("value of unknown type should always be null")
There was a problem hiding this comment.
You don't need this if any more.
There was a problem hiding this comment.
// Ideally, this function should never be invoked for unknown type.
// However, some logic rely on having a default value before the null check.
There was a problem hiding this comment.
Add checkState(type instanceof UnknownType)
There was a problem hiding this comment.
Add comment to explain why the native container type doesn't actually matter. And also explain why we picked boolean among other possible choices.
jessesleeping
force-pushed
the
unknown_type
branch
2 times, most recently
from
25f279e to
5c827c5
Compare
There was a problem hiding this comment.
In my opinion, the conflict will be minor.
What alternative do you propose? I wouldn't want to leave this as is indefinitely.
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Using boolean to represent UNKNOWN helps remove all special handling for void/Void in functions/operators implementations and bytecode generations. In fact, any native container type can represet UNKNOWN. We choose boolean becuase it's the smallest primitive type.
jessesleeping
force-pushed
the
unknown_type
branch
from
5c827c5 to
ab945ec
Compare
4 participants
Using boolean to represent
UNKNOWNhelps remove all special handlingfor void/Void in functions/operators implementations and bytecode
generations.
The changes were done by:
UnknownType.javaandUnknownOperators.java, followed by all fixes to pass all tests underpresto-main/src/test/java/com/facebook/presto/typeandpresto-main/src/test/java/com/facebook/presto/operatorvoid.classand check if it's related toUNKNOWNhandlingVoid.classand check if it's relatedpublic static Void.classand check if it's return value of certain SQL functions/operators@SqlType\([^\)]*\) Voidand check if it's parameters of certain SQL functions/operators