[jira] [Comment Edited] (CALCITE-4372) Correct specification getCumulativeCost and getNonCumulativeCost so it should always produce non-nullable value

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

    [ https://issues.apache.org/jira/browse/CALCITE-4372?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17225019#comment-17225019 ] 

Julian Hyde edited comment on CALCITE-4372 at 11/3/20, 12:15 AM:
-----------------------------------------------------------------

It is standard across all metadata types that 'unknown' is represented by 'null'. It doesn't make sense for one kind of metadata to have a different standard.

I accept that there are bugs in the implementation: we don't handle nulls every place that we should. I think the solution is to handle the nulls. Maybe return an Optional, if it helps remind us.

I don't like Infinity, because it is another value that has strange semantics that need to be anticipated and handled.

In sum, I don't like either option. Option 1 is pretty much what we have today, but we've formalized in the doc so we can say 'I told you so'. Option 2 doesn't solve the problem for other metadata types and might just be substituting NPEs with other problems like explicit overthrows and silent underflows.

I propose Option 3. Change the result type to {{Optional<Double>}}. Yes, it's a breaking change. Each client that calls the API will have to change their code to handle {{Optional.EMPTY}}. That's a feature, not a bug. And we can apply the same pattern to all metadata types.


was (Author: julianhyde):
It is standard across all metadata types that 'unknown' is represented by 'null'. It doesn't make sense for one kind of metadata to have a different standard.

I accept that there are bugs in the implementation: we don't handle nulls every place that we should. I think the solution is to handle the nulls. Maybe return an Optional, if it helps remind us.

I don't like Infinity, because it is another value that has strange semantics that need to be anticipated and handled.

> Correct specification getCumulativeCost and getNonCumulativeCost so it should always produce non-nullable value
> ---------------------------------------------------------------------------------------------------------------
>
>                 Key: CALCITE-4372
>                 URL: https://issues.apache.org/jira/browse/CALCITE-4372
>             Project: Calcite
>          Issue Type: Sub-task
>          Components: core
>    Affects Versions: 1.26.0
>            Reporter: Vladimir Sitnikov
>            Assignee: Vladimir Sitnikov
>            Priority: Major
>
> Currently javadoc says {{getNonCumulativeCost}} can return {{null}}, however, a lot of usages assume the resulting value is not null.
> For instance, FLINK-11973 is relevant here.
> Even though plugins can install custom {{getNonCumulativeCost}} handlers, we should make {{get*CumulativeCost}} non-nullable, so {{RelOptPlanner.getCost}} could be declared and implemented as non-nullable (which is the way the current code behaves)
> It does not look right if we declare that "metadata can return null", and then Volcano fails with NPE :(



--
This message was sent by Atlassian Jira
(v8.3.4#803005)