docete edited a comment on issue #9394: [FLINK-13547][table-planner-blink] Verify and correct string function's semantic for Blink planner URL: https://github.com/apache/flink/pull/9394#issuecomment-519812303 > Thanks @docete , I left some suggestions. > > 1. Remove "SUBSTR" builtin function definition and tests? Agree! Forgot to remove this non-standard function. > 2. Should "MD5" and "SHA" take SqlTypeFamily.CHARACTER instead of SqlTypeFamily.STRING ? Sure. But now both Blink planner and old planner use SqlTypeFamily.STRING. I will open [a new ticket](https://issues.apache.org/jira/browse/FLINK-13664) to trace and fix both side. > 3. Could you add a detailed description what builtin function are changed and the reason for commit "Refactor non-standard functions"? You can take [28260cd](https://github.com/apache/flink/commit/28260cdd70aee0a4169a068887feae71a81ec4c0) as an example. OK, will fix in next hours. > 4. Add `testInvalidTruncate1` and `testInvalidTruncate2` to `ScalarFunctionsValidationTest` I don't understand. U mean put the decimal related truncate tests to a single test function? > 5. We should also support truncate TableAPI. Please add the following case to `SqlExpressionTest#testArithmeticFunctions` > > ``` > testSqlApi("truncate(42.345)", "42") > testSqlApi("truncate(cast(42.345 as decimal(2, 3)), 2)", "42.34") > ``` > > Btw, please add a prefix to the append commit message to describe which commit should be squashed into. For example: "fixup concat commit: fix test case". So that the committer can help to squash your commits easier. OK, will fix in next hours.
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
