[GitHub] [flink] godfreyhe commented on a diff in pull request #22031: [FLINK-31239][hive] Fix native sum function can't get the corrected value when the argument type is string

["godfreyhe (via GitHub)" <gi...@apache.org>]

godfreyhe commented on code in PR #22031:
URL: https://github.com/apache/flink/pull/22031#discussion_r1123080762


##########
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/functions/hive/HiveSumAggFunction.java:
##########
@@ -88,27 +97,27 @@ public Expression[] retractExpressions() {
 
     @Override
     public Expression[] mergeExpressions() {
+        Expression coalesceSum = coalesce(sum, zero);
         return new Expression[] {
             /* sum = */ ifThenElse(
                     isNull(mergeOperand(sum)),
-                    sum,
-                    ifThenElse(
-                            isNull(sum),
-                            mergeOperand(sum),
-                            adjustedPlus(getResultType(), sum, mergeOperand(sum))))
+                    coalesceSum,
+                    adjustedPlus(getResultType(), coalesceSum, mergeOperand(sum))),
+            and(isEmpty, mergeOperand(isEmpty))
         };
     }
 
     @Override
     public Expression getValueExpression() {
-        return sum;
+        return ifThenElse(isTrue(isEmpty), nullOf(getResultType()), sum);

Review Comment:
   what's the hive behavior when input is empty?
   
   btw, please add a test to cover this case



##########
docs/content.zh/docs/connectors/table/hive/hive_functions.md:
##########
@@ -100,6 +100,7 @@ the option `table.exec.hive.native-agg-function.enabled`, which brings significa
 </table>
 
 <span class="label label-danger">Attention</span> The ability of the native aggregation functions doesn't fully align with Hive built-in aggregation functions now, for example, some data types are not supported. If performance is not a bottleneck, you don't need to turn on this option.
+In addition, `table.exec.hive.native-agg-function.enabled` option can't be turned on per job when using it via SqlClient, currently, only the module level is supported. Users should turn on this option first and then load HiveModule. This issue will be fixed in [FLINK-31193](https://issues.apache.org/jira/browse/FLINK-31193).

Review Comment:
   I think we do not need add the issue number in the doc.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org