[
https://issues.apache.org/jira/browse/CALCITE-4771?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17702148#comment-17702148
]
Julian Hyde edited comment on CALCITE-4771 at 3/18/23 5:24 PM:
---------------------------------------------------------------
Reviewing PR at 46a5ace25e:
* Move the conformance check from the parser to the validator;
* Once that's moved, the 'if' block in the parser can be made more concise
* Add a blank line before method {{expressionHandlingSafe}}
* Add blank line before method {{translateCast}}
* The line "body), Expressions.catch_(" is weird, and difficult to read. Don't
end a function call and start a new one on the same line. Indentation should
always match nesting.
* Method {{translateCastToTime}} would be clearer if each branch had return,
rather than assigning to a variable. Maybe other methods too.
* Remove blank line between {{makeCast}} and its javadoc
* In RexBuilder, javadoc shouldn't mention SAFE_CAST explicitly. It will be
confusing when TRY_CAST is implemented. {{boolean safe}} is probably better
than {{SqlKind kind}}.
* Can you somehow reduce the number of {{makeCast}} methods? Too many overloads
isn't helpful to anyone. Deprecate an existing method if you need to.
* Calling deriveType with true doesn't seem quite right. If they write
'SAFE_CAST(e, INTEGER)' it will change the data type spec as if they wrote
'SAFE_CAST(e, NULLABLE INTEGER)' which is not quite true. The result type is
nullable, but the data type isn't.
Making safeCast a conformance method is a neat idea. However, it makes things
more complicated to configure (and we would need to add another conformance
method for TRY_CAST). The natural way to configure this is via library. Can we
do that instead?
Overall, try to limit the number of times you mention SAFE_CAST in this PR. We
know that TRY_CAST change is coming down the pike, we know that the SAFE_CAST
terminology only makes sense to BigQuery people, so let's not do work now that
will have to be undone later.
was (Author: julianhyde):
Reviewing PR at 46a5ace25e:
* Move the conformance check from the parser to the validator;
* Once that's moved, the 'if' block in the parser can be made more concise
* Add a blank line before method {{expressionHandlingSafe}}
* Add blank line before method {{translateCast}}
* The line "body), Expressions.catch_(" is weird, and difficult to read. Don't
end a function call and start a new one on the same line. Indentation should
always match nesting.
* Method {{translateCastToTime}} would be clearer if each branch had return,
rather than assigning to a variable. Maybe other methods too.
* Remove blank line between {{makeCast}} and its javadoc
* In RexBuilder, javadoc shouldn't mention SAFE_CAST explicitly. It will be
confusing when TRY_CAST is implemented. {{boolean safe}} is probably better
than {{SqlKind kind}}.
* Can you somehow reduce the number of {{makeCast}} methods? Too many overloads
isn't helpful to anyone. Deprecate an existing method if you need to.
Making safeCast a conformance method is a neat idea. However, it makes things
more complicated to configure (and we would need to add another conformance
method for TRY_CAST). The natural way to configure this is via library. Can we
do that instead?
> change the value of the CAST function to be nullable
> -----------------------------------------------------
>
> Key: CALCITE-4771
> URL: https://issues.apache.org/jira/browse/CALCITE-4771
> Project: Calcite
> Issue Type: Improvement
> Reporter: xuyang
> Assignee: Zou
> Priority: Major
> Attachments: image-2021-09-16-11-43-55-743.png
>
>
> In the sql "SELECT CAST('haha' AS INT)",the value the function CAST returns
> will be parsed into NOT NULL, because when parsing, the type CAST returns is
> from the INT and the nullable attribute is from the 'haha', which doesn't
> consider the condition that parsing a string to an int could be invalid and
> return NULL values.
> I think there are two ways to improve this question:
> * One is to change the value of the CAST function to be nullable, which
> avoids the invalid parsing.
> * The other way is to introduce a function named TRY_CAST, which is used in
> SQL Server.If the parsing fails, TRY_CAST will return NULL instead of throws
> exception that a NOT NULL field will be set with our unexpected value NULL.
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
