Implement ARRAY_NORMALIZE function#16159
Conversation
a087f38 to
254cb32
Compare
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/operator/scalar/TestArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
a2b1a3e to
f50c6ed
Compare
3ef8fe2 to
0c51328
Compare
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/operator/scalar/ArrayNormalizeFunction.java
Outdated
Show resolved
Hide resolved
Normalizes array ``x`` by dividing each element by the p-norm of the array. It is equivalent to `TRANSFORM(array, v -> v / REDUCE(array, 0, (a, v) -> a + POW(ABS(v), p), a -> POW(a, 1 / p))`. But the reduce part is only executed once to improve performance. Returns null if the array is null or there are null array elements.
| double pNorm = 0; | ||
| for (int i = 0; i < elementCount; i++) { | ||
| if (block.isNull(i)) { | ||
| return null; |
There was a problem hiding this comment.
I wonder if this should work like an "Aggregation" so ignore nulls?
There was a problem hiding this comment.
Good question. Before I prepared this commit, I discussed with erikbrinkman who requested this function in issue #16134. There was no specific requirement for how to handle null element.
Technically, this is not an Aggregation function as it returns an array instead of a scalar value. Thus there is no absolute right and wrong answers for this custom function. We just need to make sure the function comment states the behavior correctly. If you think ignoring nulls is better, I am happy to change this behavior.
| return blockBuilder.build(); | ||
| } | ||
|
|
||
| private interface ValueAccessor |
There was a problem hiding this comment.
This particular thing of every operation adding their own classes for typed accessors is becoming too much code bloat. See if you can make it some kind of utility class at the top level. For example, I had to do something similar in MapUnionSum:
There was a problem hiding this comment.
Totally agree. I took a closer look at the Type interface. I think it would be possible to move the type specific accessors into the subclasses of Types.
There was a problem hiding this comment.
Prepared #16209 to address this. Please see if the change makes sense.
fixes #16134
Normalizes array
xby dividing each element by the p-norm of the array.It is equivalent to
TRANSFORM(array, v -> v / REDUCE(array, 0, (a, v) -> a + POW(ABS(v), p), a -> POW(a, 1 / p)).
But the reduce part is only executed once to improve performance.
Returns null if the array is null or there are null array elements.
Test plan