[Isthmus] Specify failure behavior on cast

[westonpace]

The field Cast::failure_behavior has no listed default and yet Isthmus does not specify a value which forces consumers to guess.

[mbwhite]

Hello - I was just looking at this as it's occurred in some testing I was doing (and had to manually modify the substrait plans to workaround it)

Expressions is created here

  public static Function<TypeConverter, SimpleCallConverter> CAST =
      typeConverter ->
          (call, visitor) -> {
            if (call.getKind() != SqlKind.CAST) {
              return null;
            }

            return ExpressionCreator.cast(
                typeConverter.toSubstrait(call.getType()),
                visitor.apply(call.getOperands().get(0)));

          };

Does it make sense to add a field/method on the TypeConverter to specify the error condition? Extra field needs to be added to the ExpressionCreate.cast - question is where to get this from...

[westonpace]

My guess (which could be very wrong given how little I know about isthmus) is that it call.getKind() == SqlKind.CAST implies FAILURE_BEHAVIOR_THROW_EXCEPTION and call.getKind() == SqlKind.SAFE_CAST implies FAILURE_BEHAVIOR_RETURN_NULL. Although, from the snippet you provided, it looks like SAFE_CAST isn't being handled here anyways.

[mbwhite]

Admit I've not heard of a SAFE_CAST might be the version of calcite-sql I've got. Would make sense to me to have the default be throw an exception.

[vbarua]

TIL about SqlKind.SAFE_CAST vs SqlKind.CAST. I think Weston has it correct in terms of the mappings.

@mbwhite if you were to make a PR for this I'd be happy to review it.

[mbwhite]

Thanks @vbarua

To get the SqlKind.SAFE_CAST - that needs Calcite 1.35 (implemented here) However 1.34 also had a change to the external API

I've included both in the PR as they are very small changes; anything much larger though I'd prefer to do as a separate change.