[jira] [Comment Edited] (CALCITE-4771) change the value of the CAST function to be nullable

["Julian Hyde (Jira)" <ji...@apache.org>]

    [ 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)