feat: support new IntervalCompound and IntervalDay update

[Blizzara]

Bumps substrait to latest to get substrait-io/substrait#665 and addresses the breaks, adding IntervalCompound type and literal and adapting IntervalDay type and literal to have precision. For IntervalDay, backwards-compatibility is provided for reading existing protobufs using microseconds, but not for generating new protobufs, ie new plans will always use precision and subseconds.

Requires substrait-io/substrait#687 ✅

CSV support from substrait-io/substrait#646 is not implemented yet

[Blizzara]

@vbarua @jacques-n even with the fixed functions_datetime.yml (see substrait-io/substrait#687), substrait-java fails to read the extensions, caused by the additions to functions_arithmetic_decimal.yaml that have form of decimal<P1,0>. The error is

Caused by: com.fasterxml.jackson.databind.JsonMappingException: Unable to parse string DECIMAL<P1,0>
 at [Source: (sun.net.www.protocol.jar.JarURLConnection$JarURLInputStream); line: 120, column: 20] (through reference chain: io.substrait.extension.ImmutableSimpleExtension$ExtensionSignatures$Json["scalar_functions"]->java.util.ArrayList[6]->io.substrait.extension.ImmutableSimpleExtension$ScalarFunction$Json["impls"]->java.util.ArrayList[0]->io.substrait.extension.ImmutableSimpleExtension$ScalarFunctionVariant$Json["args"]->java.util.ArrayList[0]->io.substrait.extension.ImmutableSimpleExtension$ValueArgument$Json["value"])
	at app//com.fasterxml.jackson.databind.JsonMappingException.from(JsonMappingException.java:276)
...
Caused by: java.lang.UnsupportedOperationException: This construct can only be used in Type Expressions.
	at io.substrait.type.parser.ParseToPojo$Visitor.checkExpression(ParseToPojo.java:74)
	at io.substrait.type.parser.ParseToPojo$Visitor.visitDecimal(ParseToPojo.java:264)
	at io.substrait.type.parser.ParseToPojo$Visitor.visitDecimal(ParseToPojo.java:36)

so coming from

substrait-java/core/src/main/java/io/substrait/type/parser/ParseToPojo.java

Line 264 in 545e204

checkExpression();

. I guess it fails as the logic above expects both parameters to be either string or int, but isn't suited for case where one is string and other is int. Could you please help me with how to fix it?

[vbarua]

Running this locally, the Unable to parse string DECIMAL<P1,0> error seems to be coming from

substrait-java/core/src/main/java/io/substrait/type/parser/ParseToPojo.java

Lines 222 to 238 in d625648

	@Override
	public TypeExpression visitDecimal(final SubstraitTypeParser.DecimalContext ctx) {
	boolean nullable = ctx.isnull != null;
	Object precision = i(ctx.precision);
	Object scale = i(ctx.scale);
	if (precision instanceof Integer && scale instanceof Integer) {
	return withNull(nullable).decimal((int) precision, (int) scale);
	}
	if (precision instanceof String && scale instanceof String) {
	checkParameterizedOrExpression();
	return withNullP(nullable).decimalE((String) precision, (String) scale);
	}
	checkExpression();
	return withNullE(nullable).decimalE(ctx.precision.accept(this), ctx.scale.accept(this));
	}

as you pointed out.

the logic above expects both parameters to be either string or int, but isn't suited for case where one is string and other is int.

It think this logic is too strict and we should make it looser. DECIMAL<P1,0> seems like a perfectly reasonable signature we should allow.

@jacques-n what do you think?

[andrew-coleman]

These two branches produce the same number don't they?

[Blizzara]

Yes, but with integer operations

[andrew-coleman]

@vbarua @Blizzara to continue the discussion....

It think this logic is too strict and we should make it looser. DECIMAL<P1,0> seems like a perfectly reasonable signature we should allow.

@jacques-n what do you think?

I'm really keen we get this PR merged so that we can move forward with other features that require the latest protobuf additions. I'm not sure what the 'proper' fix is for this, but perhaps we do an interim fix that passes the tests and defer the proper fix to a later PR? The following solution worked for me:

andrew-coleman@4501a78

Feel free to cherrypick this commit for this PR, if you wish.

Thanks!

[vbarua]

meta: I wonder if we can't just re-use the Java visitor here. Not for this PR though.

[Blizzara]

Yeaah frankly I'm overall not a huge fan of the way this lib works, there feels to be too much hand-rolled replication of the proto classes. I guess the reason is the proto classes are not very ergonomic to work with, but then it leads to these manual boilerplate things where there's a high risk of bugs and missing features..

[vbarua]

I agree that the manual boilerplate transformations are easy to screw up, but the improved ergonomics for building and working with plans outweigh that for me. There's a lot of transformations we perform over substrait trees that I think would be significantly harder over the generated protobuf. I actually find myself missing the niceness of the Java POJOs in other language.

[vbarua]

I think the changes @andrew-coleman proposed in #288 (comment) seem like a good fix for the test failures.

Aside from that, there's one piece of handling that is wrong in the ProtoTypeConverter that I I think we should address as well.

[Blizzara]

I think the changes @andrew-coleman proposed in #288 (comment) seem like a good fix for the test failures.

I pulled those in (thanks @andrew-coleman !). However I'm not sure if they do the right thing - I guess they make the "0" treated as a parameter name, like "P" or "S" would be? Not sure, maybe there is something later on that turns the number-string into a proper number. Anyways it does make the tests pass so I'm fine with it to unblock this PR 🤷

Aside from that, there's one piece of handling that is wrong in the ProtoTypeConverter that I I think we should address as well.

Thanks, fixed!

[jacques-n]

DECIMAL<P1,0> seems like a perfectly reasonable signature we should allow.

Agreed