[FLINK-29722][hive] Supports native hive max function for hive dialect #21605
Conversation
|
cc @luoyuxia |
|
cc @beyond1920 |
| @Override | ||
| public void setArguments(CallContext callContext) { | ||
| if (resultType == null) { | ||
| // check argument type firstly |
There was a problem hiding this comment.
check the count of argument num?
| || logicalType.is(LogicalTypeRoot.ROW)) { | ||
| throw new TableException( | ||
| String.format( | ||
| "Hive native max aggregate function does not support type: '%s' now. Please re-check the data type.", |
There was a problem hiding this comment.
May need to remind user to fall back to hive's implementation.
There was a problem hiding this comment.
Only after FLINK-30579 merged can we fallback to hive implementation.
| List<Row> result8 = | ||
| CollectionUtil.iteratorToList( | ||
| tableEnv.executeSql("select max(ts) from test_max").collect()); | ||
| assertThat(result8.toString()).isEqualTo("[+I[2021-08-10T16:26:33.400]]"); |
There was a problem hiding this comment.
Please verify the assertion in Hive3 as there's some difference in timestamp type between Hive2 and Hive3.
You can verify it in your local.
There was a problem hiding this comment.
I've run it use Hive3, the test passed.
|
@flinkbot run azure |
|
ping @luoyuxia, please see it again. |
|
@lsyldliu Thanks for the contribution. LGTM |
| || logicalType.is(LogicalTypeRoot.ROW)) { | ||
| throw new TableException( | ||
| String.format( | ||
| "Native hive %s aggregate function does not support type: %s. Please set option '%s' to false.", |
There was a problem hiding this comment.
nit: Will it better to tell why set the option to false? Something like Please set option '%s' to false to fall back to Hive's own %s aggregate?
It depends on you, current exception message also looks good to me.
|
@flinkbot run azure |
What is the purpose of the change
Supports native hive max function for hive dialect. Due to flink doesn't support compare array and row type, so this implementation also doesn't max array and row type, but hive udaf supports it.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation