ARROW-12861: [C++][Compute] Add sign function kernels

[edponce]

This PR adds the sign function to the compute layer as a unary scalar function.

• Numeric inputs result in any of (-1,0,1)
• +/-0 input returns 0
• Infinity is treated as a signed number
• NaN input returns NaN

[github-actions]

https://issues.apache.org/jira/browse/ARROW-12861

[edponce]

The sign function is an arithmetic function, therefore, I added its option allow_signed_zero to the ArithmeticOptions class.
Is this valid? given that sign does not overflows so it will not make use of check_overflow and currently, other compute functions do not use the allow_signed_zero option.

[edponce]

The test for double type failed in AMD64-MinGW32 with a RapidJSON error message "Number too big to be stored in double". I am not the first to encounter this error from RapidJSON. I disabled the tests that use std::numeric_limits<double>::max() to check if this is the triggering case.

Indeed this was the case. Not sure of a current solution for this.

[bkietz]

I'd say that even if this could be called an arithmetic function it's not ideal to require ArithmeticOptions to carry fields which only apply to sign. Please write SignOptions instead

[pitrou]

Can you explain in which context this function is useful? Furthermore, can you explain what the purpose is of having two different variants?

[edponce]

While working on the rounding functions, I make use of the signedness of the input for specific rounding modes (e.g., round-half-away-from-zero). After making sign utility functions, I found that many database systems provide such a function (although I do not have a particular use case). The option for treating zero as a signed value was suggested in issue discussion.

[pitrou]

@nealrichardson What do you think? Is this useful?

[nealrichardson]

I wouldn't say it's the most useful kernel to be adding, if I were prioritizing kernels to write, but it is something that people do. In R for example there is a sign() function: https://stat.ethz.ch/R-manual/R-devel/library/base/html/sign.html, and it's a thing in SQL too.

[ianmcook]

It's a thing in NumPy too https://numpy.org/doc/stable/reference/generated/numpy.sign.html

[pitrou]

Ok, all three versions of the sign function (Python, R, SQL) return (0,-1,+1), so this function should probably do the same.

[pitrou]

Should add a note explaining how the output is computed. You can look at other sections for examples.

[edponce]

I added a "Notes" column to the table, and included notes describing the output values and special case of 0.

[pitrou]

These tests which tweak the validity bits by hand are only meaningful if the kernel may return an error, which is not the case here.

[edponce]

Thanks for the explanation. These tests were removed.

[pitrou]

Please remove commented out code.

[edponce]

The Sign kernels were created with a fixed output type (Int8) using a new kernel generator dispatcher but the tests for DispatchBest fail. The output value from DispatchBest is the same as its input and the input/output values for DispatchExact are always int8. Could someone help with this issue?

[pitrou]

@bkietz may have the answer to your question, @edponce .

[bkietz]

Suggested change

	* \(2) Similar to ``sign`` but zero input is considered as a signed number.
	* \(2) Similar to ``sign`` but zero is considered signed for floating point inputs.

[edponce]

The sign_with_signed_zero variant treats 0 as a positive value for integral types, but either -0 or +0 for floating-point types.

[edponce]

Hmm...I agree with this change. The sign_with_signed_zero variant only applies to floating-point numbers resulting in either (-1,1). For integral values, the result will be either of (-1,0,1).

[bkietz]

This seems unnecessarily general, especially given we have only "sign". Additionally, note that CheckDispatchBest does not assert the output type: it asserts the type to which arguments must be implicitly cast:

Suggested change

	// Functions with fixed output type
	structFuncAndOutType {
	std::string name;
	std::shared_ptr<DataType> out_type;
	};
	FuncAndOutType funcs[] = {{"sign", int8()}};
	for (constauto& func : funcs) {
	for (constauto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
	uint64(), float32(), float64()}) {
	CheckDispatchBest(func.name, {ty}, {func.out_type});
	CheckDispatchBest(func.name, {dictionary(int8(), ty)}, {func.out_type});
	}
	}
	// Sign always outputs to int8
	for (std::string name : {"sign", "sign_with_negative_zero"}) {
	for (constauto& ty : {int8(), int16(), int32(), int64(), uint8(), uint16(), uint32(),
	uint64(), float32(), float64()}) {
	CheckDispatchBest(name, {ty}, {ty});
	CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty});
	}
	}

In the example above, CheckDispatchBest(name, {ty}, {int8()}); would imply that we must first cast ty -> int8() inputs before invoking the kernel, which is incorrect. Compare this with CheckDispatchBest(name, {dictionary(int8(), ty)}, {ty}); which asserts that we can't directly extract the sign bit from dictionary encoded data; encoded arrays must first be decoded for the kernel to operate on them.

[pitrou]

I'm still not sure why we need both variants. Is this a user request? @ianmcook ?

[pitrou]

Note that we could expose a copysign instead of sign_with_signed_zero, which would be more generic.
(sign_with_signed_zero(x) would simply be copysign(x, 1), AFAICT)

[edponce]

@pitrou The sign kernel is to be used by the rounding function. I agree with the fact that sign_with_signed_zero is equivalent to copysign. Actually, copysign is the variant needed for rounding function.

Nevertheless, sign is a common function exposed in frontend tools, so I consider it worthwhile to provide.

[edponce]

Also, I consider that copysign should not be exposed as a compute function but rather used internally, and instead we should expose sign and sign_with_signed_zero (implementations may use copysign(x, 1). The rationale for this is that copysign(x, 1) is not common in any SQL, dataframes library but sign is.

A use case of sign is to build a custom ranking based on the signedness of values.
For example, consider ranking values relative to a constant value (e.g., 3):

SELECT CASE WHEN @a < 3 THEN 0
WHEN @a = 3 THEN 1
WHEN @a > 3 THEN 2
END

is not supported because expressions are not allowed in CASE-WHEN constructs, so a workaround is

SELECT CASE SIGN(@a - 3)
WHEN -1 THEN 0
WHEN 0 THEN 1
WHEN 1 THEN 2
END

[pitrou]

Please, can we answer the original question? Why are two variants needed?

[edponce]

After careful thought, there is not a general need for having two variants. The only case where I see that sign_with_signed_zero can be relevant is if data stored is used for calculus-related operations (e.g., integral limits).
I will remove the sign_with_signed_zero variant.

[bkietz]

LGTM, thanks!