tanclary commented on PR #3147:
URL: https://github.com/apache/calcite/pull/3147#issuecomment-1625884550
> Is the new function class `SqlContainsSubstrFunction` necessary?
>
> Thanks for adding javadoc in SqlFunctions. For future reference, `{@code
... }` is slightly preferred over `<code> ... </code>` because it handles some
HTML escaping. (Of course you can't use it if you actually want HTML markup
e.g. `<b>`and `<br>`.)
>
> I'd rename `enum JsonScopeValue` to `enum JsonScope`. Every class holds
values.
>
> Make `normalize` private.
I think right now the class is not fully necessary, but I think there are
other `CONTAINS` functions (such as `REGEXP_CONTAINS`) that could make having
its own dedicated class useful in the future. Additionally, there is currently
an exception that gets thrown from `validateColumnListParams` if we just use
`SqlBasicFunction.create` instead. The `#deriveType` override avoids this. Let
me know what you think.
Good to know about the comments.
I amended to address your 3rd and 4th points.
--
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: commits-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
