You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@calcite.apache.org by Vladimir Sitnikov <si...@gmail.com> on 2020/12/09 07:26:19 UTC
[CALCITE-4431] Use requireNonNull(var, "var") instead of
requireNonNull(var) for better error messages
Hi,
I suggest we use requireNonNull(var, "var") pattern rather
than requireNonNull(var).
Then error message would include which variable/field turned out to be null.
It would make a big difference in case a constructor verifies multiple
parameters like in Correlate:
protected Correlate(
RelOptCluster cluster,
RelTraitSet traitSet,
RelNode left,
RelNode right,
CorrelationId correlationId,
ImmutableBitSet requiredColumns,
JoinRelType joinType) {
super(cluster, traitSet, left, right);
assert !joinType.generatesNullsOnLeft() : "Correlate has invalid join
type " + joinType;
this.joinType = requireNonNull(joinType);
this.correlationId = requireNonNull(correlationId);
this.requiredColumns = requireNonNull(requiredColumns);
If a user makes a mistake and passes null to joinType or correlationId,
then Calcite would throw NPE, however, it would be hard to tell which
parameter was null.
Line numbers are known to drift over time.
I suggest we use the following pattern consistently:
this.joinType = requireNonNull(joinType, "joinType");
this.correlationId = requireNonNull(correlationId, "correlationId");
this.requiredColumns = requireNonNull(requiredColumns,
"requiredColumns");
The PR is https://github.com/apache/calcite/pull/2293
The suggested change automatically aligns single-word message with the
first argument to requireNonNull.
In other words, "./gradlew style" command would insert/fix the messages
automatically.
I assume modern IDEs enable developers to configure var =>
requireNonNull(var, "var") expansion (I use it in IDEA).
Currently we have
545 usages of requireNonNull(var) <-- this is to be updated to (var, "var")
359 usages of requireNonNull(var, "var")
37 usages of requireNonNull(var, () -> ..)
33 usages of requireNonNull(var, "custom message")
If no-one objects within three days, I'll assume lazy consensus and commit
it.
Vladimir
Re: [CALCITE-4431] Use requireNonNull(var, "var") instead of
requireNonNull(var) for better error messages
Posted by Vladimir Sitnikov <si...@gmail.com>.
Thank you, Stamatis and Haisheng for the feedback.
I will rebase the PR soon.
Vladimir
Re: [CALCITE-4431] Use requireNonNull(var, "var") instead of requireNonNull(var) for better error messages
Posted by Haisheng Yuan <hy...@apache.org>.
Makes sense.
On 2020/12/09 22:44:35, Stamatis Zampetakis <za...@gmail.com> wrote:
> Sounds reasonable and shares some goals with JEP358 [1] so why not.
>
> [1] https://openjdk.java.net/jeps/358
>
> On Wed, Dec 9, 2020 at 8:26 AM Vladimir Sitnikov <
> sitnikov.vladimir@gmail.com> wrote:
>
> > Hi,
> >
> > I suggest we use requireNonNull(var, "var") pattern rather
> > than requireNonNull(var).
> >
> > Then error message would include which variable/field turned out to be
> > null.
> > It would make a big difference in case a constructor verifies multiple
> > parameters like in Correlate:
> >
> > protected Correlate(
> > RelOptCluster cluster,
> > RelTraitSet traitSet,
> > RelNode left,
> > RelNode right,
> > CorrelationId correlationId,
> > ImmutableBitSet requiredColumns,
> > JoinRelType joinType) {
> > super(cluster, traitSet, left, right);
> > assert !joinType.generatesNullsOnLeft() : "Correlate has invalid join
> > type " + joinType;
> > this.joinType = requireNonNull(joinType);
> > this.correlationId = requireNonNull(correlationId);
> > this.requiredColumns = requireNonNull(requiredColumns);
> >
> > If a user makes a mistake and passes null to joinType or correlationId,
> > then Calcite would throw NPE, however, it would be hard to tell which
> > parameter was null.
> > Line numbers are known to drift over time.
> >
> > I suggest we use the following pattern consistently:
> >
> > this.joinType = requireNonNull(joinType, "joinType");
> > this.correlationId = requireNonNull(correlationId, "correlationId");
> > this.requiredColumns = requireNonNull(requiredColumns,
> > "requiredColumns");
> >
> > The PR is https://github.com/apache/calcite/pull/2293
> >
> > The suggested change automatically aligns single-word message with the
> > first argument to requireNonNull.
> > In other words, "./gradlew style" command would insert/fix the messages
> > automatically.
> > I assume modern IDEs enable developers to configure var =>
> > requireNonNull(var, "var") expansion (I use it in IDEA).
> >
> > Currently we have
> > 545 usages of requireNonNull(var) <-- this is to be updated to (var, "var")
> > 359 usages of requireNonNull(var, "var")
> > 37 usages of requireNonNull(var, () -> ..)
> > 33 usages of requireNonNull(var, "custom message")
> >
> > If no-one objects within three days, I'll assume lazy consensus and commit
> > it.
> >
> > Vladimir
> >
>
Re: [CALCITE-4431] Use requireNonNull(var, "var") instead of
requireNonNull(var) for better error messages
Posted by Stamatis Zampetakis <za...@gmail.com>.
Sounds reasonable and shares some goals with JEP358 [1] so why not.
[1] https://openjdk.java.net/jeps/358
On Wed, Dec 9, 2020 at 8:26 AM Vladimir Sitnikov <
sitnikov.vladimir@gmail.com> wrote:
> Hi,
>
> I suggest we use requireNonNull(var, "var") pattern rather
> than requireNonNull(var).
>
> Then error message would include which variable/field turned out to be
> null.
> It would make a big difference in case a constructor verifies multiple
> parameters like in Correlate:
>
> protected Correlate(
> RelOptCluster cluster,
> RelTraitSet traitSet,
> RelNode left,
> RelNode right,
> CorrelationId correlationId,
> ImmutableBitSet requiredColumns,
> JoinRelType joinType) {
> super(cluster, traitSet, left, right);
> assert !joinType.generatesNullsOnLeft() : "Correlate has invalid join
> type " + joinType;
> this.joinType = requireNonNull(joinType);
> this.correlationId = requireNonNull(correlationId);
> this.requiredColumns = requireNonNull(requiredColumns);
>
> If a user makes a mistake and passes null to joinType or correlationId,
> then Calcite would throw NPE, however, it would be hard to tell which
> parameter was null.
> Line numbers are known to drift over time.
>
> I suggest we use the following pattern consistently:
>
> this.joinType = requireNonNull(joinType, "joinType");
> this.correlationId = requireNonNull(correlationId, "correlationId");
> this.requiredColumns = requireNonNull(requiredColumns,
> "requiredColumns");
>
> The PR is https://github.com/apache/calcite/pull/2293
>
> The suggested change automatically aligns single-word message with the
> first argument to requireNonNull.
> In other words, "./gradlew style" command would insert/fix the messages
> automatically.
> I assume modern IDEs enable developers to configure var =>
> requireNonNull(var, "var") expansion (I use it in IDEA).
>
> Currently we have
> 545 usages of requireNonNull(var) <-- this is to be updated to (var, "var")
> 359 usages of requireNonNull(var, "var")
> 37 usages of requireNonNull(var, () -> ..)
> 33 usages of requireNonNull(var, "custom message")
>
> If no-one objects within three days, I'll assume lazy consensus and commit
> it.
>
> Vladimir
>