[jira] [Comment Edited] (CALCITE-5477) Prefer Util.checkArgument over Preconditions.checkArgument

["Gian Merlino (Jira)" <ji...@apache.org>]

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

Gian Merlino edited comment on CALCITE-5477 at 1/17/23 8:34 PM:
----------------------------------------------------------------

The situation is a little awkward due to the new overloads. I believe the possible solutions are:
 - The one in the linked PR, involving an additional level of indirection.
 - Make release builds of Calcite using an older version of Guava (prior to 20.0). People could always drop in a newer version instead, but this may be annoying for downstream consumers.
 - Avoiding the new overloads, using {{forbidden-apis}} plus some code changes.

With the third approach, code changes are needed because the new overloads capture calls that previously went to a varags {{Object...}} method. An example of the problem is this call in DateString:

{code:java}
Preconditions.checkArgument(PATTERN.matcher(v).matches(),
    "Invalid date format:", v);
{code}

{{v}} is a {{{}Object{}}}. If built against Guava prior to 20.0, this goes to {{{}checkArgument(boolean, String, Object...){}}}. However, when building against later versions of Guava, this goes to {{{}checkArgument(boolean, String, Object){}}}. This method doesn't exist in Guava prior to 20.0, which leads to a method-not-found error at runtime.

So, to solve that, all such calls would need to be replaced with explicit {{Object[]}} creation, such as:
{code:java}
Preconditions.checkArgument(PATTERN.matcher(v).matches(),
    "Invalid date format:", new Object[]{v});
{code}

Definitely open to any solution that seems best.


was (Author: gian):
The situation is a little awkward due to the new overloads. I believe the possible solutions are:
 - The one in the linked PR, involving an additional level of indirection.
 - Build release builds of Calcite using an older version of Guava (prior to 20.0). People could always drop in a newer version instead, but this may be annoying for downstream consumers.
 - Avoiding the new overloads, using {{forbidden-apis}} plus some code changes.

With the third approach, code changes are needed because the new overloads capture calls that previously went to a varags {{Object...}} method. An example of the problem is this call in DateString:

{code:java}
Preconditions.checkArgument(PATTERN.matcher(v).matches(),
    "Invalid date format:", v);
{code}

{{v}} is a {{{}Object{}}}. If built against Guava prior to 20.0, this goes to {{{}checkArgument(boolean, String, Object...){}}}. However, when building against later versions of Guava, this goes to {{{}checkArgument(boolean, String, Object){}}}. This method doesn't exist in Guava prior to 20.0, which leads to a method-not-found error at runtime.

So, to solve that, all such calls would need to be replaced with explicit {{Object[]}} creation, such as:
{code:java}
Preconditions.checkArgument(PATTERN.matcher(v).matches(),
    "Invalid date format:", new Object[]{v});
{code}

Definitely open to any solution that seems best.

> Prefer Util.checkArgument over Preconditions.checkArgument
> ----------------------------------------------------------
>
>                 Key: CALCITE-5477
>                 URL: https://issues.apache.org/jira/browse/CALCITE-5477
>             Project: Calcite
>          Issue Type: Improvement
>            Reporter: Gian Merlino
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 20m
>  Remaining Estimate: 0h
>
> Related to CALCITE-5428.
> Preconditions.checkArgument gained new overloads in Guava 20.0, which prevents code built using Guava 20.0 from running properly with earlier versions of Guava. (When building against a later version of Guava, as Calcite does by default, the Java compiler generates calls to methods that don't exist in those earlier versions.)
> Example: in Guava 20.0 there is {{checkArgument(boolean, String, int)}}; in earlier versions this would be handled by {{checkArgument(boolean, String, Object...)}}.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)