feat(extensions): support int arguments with std_dev and variance functions

[nielspardon]

This PR depends on #1010 and #1011 since it uses the new enum argument syntax in the test cases and adds additional generated test cases using integer arguments to the generated test cases in #1011.

The main changes for this PR are in commit 714d363.

This PR adds new function signatures for std_dev and variance which accept integer arguments. Most SQL systems support any numeric arguments not just floating point arguments as the current Substrait function signatures suggest.

Integer arguments are used in TPC-DS and are required so we can correctly convert TPC-DS queries with isthmus in substrait-java. See: substrait-io/substrait-java#68

---

This change is 

[mbwhite]

LGTM - good to get the complete implementation - and following the pattern of avg

[yongchul]

+1 for supporting integral types.

[nielspardon]

rebased on latest #1011 to remove the deprecation

[vbarua]

Do we actually need to support calls likestd_dev(<i8>), or should we pushing producers to generate std_dev(<i8>::fp64) to force them to capture their type promotion logic.

following the pattern of avg

This is slightly different from the avg case IMO. The avg functions output the same numeric type as the input. So avg:i8 outputs i8, avg:i16 outputs i16, etc.

For the std_dev and variance functions with numeric inputs, the output is always fp64. There is no difference in output types that we need to capture.

[nielspardon]

For the std_dev and variance functions with numeric inputs, the output is always fp64. There is no difference in output types that we need to capture.

Currently, the fp32 argument versions also returns an fp32 result:

substrait/extensions/functions_arithmetic.yaml

Lines 1395 to 1404 in 7b39d4c

	- args:
	- name: x
	value: fp32
	options:
	rounding:
	values: [ TIE_TO_EVEN, TIE_AWAY_FROM_ZERO, TRUNCATE, CEILING, FLOOR ]
	distribution:
	values: [ SAMPLE, POPULATION]
	nullability: DECLARED_OUTPUT
	return: fp32?

substrait/extensions/functions_arithmetic.yaml

Lines 1418 to 1427 in 7b39d4c

	- args:
	- name: x
	value: fp32
	options:
	rounding:
	values: [ TIE_TO_EVEN, TIE_AWAY_FROM_ZERO, TRUNCATE, CEILING, FLOOR ]
	distribution:
	values: [ SAMPLE, POPULATION]
	nullability: DECLARED_OUTPUT
	return: fp32?

[nielspardon]

Do we actually need to support calls likestd_dev(<i8>), or should we pushing producers to generate std_dev(<i8>::fp64) to force them to capture their type promotion logic.

I think not offering function signatures with all numeric types makes it only less convenient for Substrait adopters since they need to add and parse extra cast expressions which they otherwise would not need. Sure, we could force them to do so but then I also feel that the Substrait specification is under-specified on the expected casting behavior like e.g. when casting from fp32 to i64 does it round up or down?

[vbarua]

Currently, the fp32 argument versions also returns an fp32 result:

Should have been clearer. The integer args versions all return fp64.

I also feel that the Substrait specification is under-specified on the expected casting behavior like e.g. when casting from fp32 to i64 does it round up or down?

I agree actually, but that sounds like something worth pinning down as well.

I think not offering function signatures with all numeric types makes it only less convenient for Substrait adopters since they need to add and parse extra cast expressions which they otherwise would not need.

It makes the producers life more difficult, but lessens the burden on the consumer because the surface area of functions to implement/support is smaller. I haven't figured out a good criteria to express when we should push for a plan producer to cast, but I don't think think we need to provide 1-1 functions for everything in SQL. This actually might be a case where it's worth it, but forcing producers to cast is also a mechanism to get them to capture their type coercion behaviors explicitly.

[nielspardon]

I also feel that the Substrait specification is under-specified on the expected casting behavior like e.g. when casting from fp32 to i64 does it round up or down?

I agree actually, but that sounds like something worth pinning down as well.

created an issue for the casting docs improvement: #1023

[nielspardon]

It makes the producers life more difficult, but lessens the burden on the consumer because the surface area of functions to implement/support is smaller. I haven't figured out a good criteria to express when we should push for a plan producer to cast, but I don't think think we need to provide 1-1 functions for everything in SQL. This actually might be a case where it's worth it, but forcing producers to cast is also a mechanism to get them to capture their type coercion behaviors explicitly.

If you wanted to enforce such a behavior you would have to analyze all of the existing function signatures and deprecate/remove the ones that are "too convenient", not forcing producers to expose their type coercion behavior.

Also this will require a major rewrite of the isthmus aggregate function mapping (at least for std_dev, variance but also for scalar, windowing functions prospectively) since the way it is implemented today does not allow to explicitly coerce types using casts since in the function conversion logic we have only access to field indices of fields used in aggregate functions but not their types.

[nielspardon]

I opened a PR in substrait-java that shows how casting arguments explicitly in a plan producer (isthmus) looks like: substrait-io/substrait-java#780

[jacques-n]

It is hard to look at the current patch and confirm whether it is a breaking change. It definitely looks like one but I suppose it is possible that you moved function location around and this is net new functions. It doesn't seem like this needs to be a breaking change.

[nielspardon]

It is hard to look at the current patch and confirm whether it is a breaking change. It definitely looks like one but I suppose it is possible that you moved function location around and this is net new functions. It doesn't seem like this needs to be a breaking change.

This is mainly because it is based on other PRs that I originally posted in parallel. I have not updated this PR recently given the discussion around whether we want to have additional function signatures with int types or force users to explicitly cast non floating point arguments.
#1012 (comment)

@jacques-n What is your point of view on asking users to cast vs. offering more convenience by offering alternative function signatures accepting int arguments?

[jacques-n]

@jacques-n What is your point of view on asking users to cast vs. offering more convenience by offering alternative function signatures accepting int arguments?

I'm inclined to look at the nature of the underlying operation. I would avoid creating composite "friendly versions" of operations that are simply going to be cross-cast immediately with potential lossy-ness and errors. For example, I don't believe there are any amd64 instructions that take in integer arguments for trigonometric or statistical functions. If there are first class integer ops, then I'd be inclined to include them. I believe that was the original thinking I tried to apply when creating the functions I added.

[nielspardon]

I'm closing this PR for now in favor of adding explicit casts to floating point in the plan producer.