clintropolis opened a new pull request, #12968: URL: https://github.com/apache/druid/pull/12968
### Description This PR makes some improvements to `JSON_VALUE` behavior, which fixes some bugs and inconsistency in behavior between the nested-field virtual column and the native `json_value` expression. The first change is that the native `json_value` expression now has optional 3rd argument to specify type, which will cast all values to the specified type. This brings its behavior inline with `NestedFieldVirtualColumn`, whose selectors will cast values to the type specified by `expectedType`, regardless of the type of the underlying literal. In attempting to wire this up, I realized how strangely I had `JSON_VALUE` wired up in the SQL layer. Prior to this PR, `JsonValueOperatorConversion` was wired up in the operator table as `JSON_VALUE`, and `JsonValueAnyOperatorConversion` was added as an alias to define an undocumented `JSON_VALUE_ANY` function (which is still undocumented, it doesn't really seem intended for direct use). The `JSON_VALUE_ANY` is there because the `StandardConvertletTable` has a rule that translates `JSON_VALUE(... RETURNING type)` into `CAST(JSON_VALUE_ANY(...) AS type)`. In effect, `JsonValueOperatorConversion` was only used indirectly via `JsonValueAnyOperatorConversion` via the convertlet, since it would be translated prior to translating to native query components. This standard convertlet behavior isn't super useful because it means our operator conversion loses the type of the 'returning' which is now part of the outer cast, making it difficult to actually use the new type argument for the native expression. Taking inspiration from the standard convertlet table behavior, I've added `DruidJsonValueConvertletFactory` to do this conversion ourselves, but more appropriate for our needs. This convertlet checks the type of the `RETURNING` argument, if present, and uses new internal only operators to handle type specific casts, `JsonValueBigintOperatorConversion`, `JsonValueDoubleOperatorConversion`, and `JsonValueVarcharOperatorConversion`, and changes `JsonValueAnyOperatorConversion` into a real conversion, taking the place of `JsonValueOperatorConversion` which is now removed because it is now handled by the convertlet. These internal functions will all remain undocumented for now at least. <hr> This PR has: - [ ] been self-reviewed. - [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links. - [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader. - [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met. - [ ] added integration tests. - [ ] been tested in a test Druid cluster. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
