Allow casted literal values in SQL functions accepting literals (Part 2) #15316
Conversation
![github-advanced-security[bot]](https://github.com/avatars/in/57789?s=60&v=4){kind=small}
sql/src/main/java/org/apache/calcite/sql/type/CastedLiteralOperandTypeCheckers.java
Fixed
Show resolved
Hide resolved
|
|
||
| import java.math.BigDecimal; | ||
|
|
||
| public class CastedLiteralOperandTypeCheckers |
There was a problem hiding this comment.
Nit: What are the implications of this during calcite upgrade ?
There was a problem hiding this comment.
Or better yet move them to apache calcite ?
There was a problem hiding this comment.
Nit: What are the implications of this during the Calcite upgrade?
Since this code is a copy & paste from the Calcite, on upgrading Calcite (minor version), it'd be good to check if anything major has changed it in the code. On a major version upgrade, the interfaces and method signatures are likely to change so we'd probably have to copy & paste the new class with the desired changes.
Or better yet move them to Apache Calcite?
Casting a literal isn't a very valid operation. Either the type-checking code should allow the dynamic parameters (in Calcite), or we need a different way to replace the parameters. This is kind of a hacky fix, which allows CAST on literals using the type checker in the code. The real fix would be to allow the users to not write dynamic parameters with CAST.
The only downside with this approach, and Druid 28 is that since we are suggesting users do a CAST(? AS TYPE) while writing JDBC queries, we would need to support this method even if we fix the real reason because users might be writing the queries with an explicit cast. However, that cannot be helped by this fix. This just allows what users have been advised to pass through the Druid's functions.
|
There would be a few code coverage failures, which is expected, given that I have copied functions from Calcite. Adding a few negative tests might improve it but won't fix it completely, so I am favor of leaving them as is. |
|
The following 5 actions are failing, and all of them are related to codecov in the newly added functions. https://github.com/apache/druid/actions/runs/6744567321/job/18335247194?pr=15316 |
Description
This is a follow-up to #15282. I missed a few instances of type checkers being used which internally used the
OperandTypes.LITERAL, causing the same issue as described in the mentioned PR. This PR cleans up those usages as well, and unfortunately duplicates a fair bit of code from Calcite, to change a few calls which define whether or not casted literals can be used or not.Release note
Key changed/added classes in this PR
MyFooOurBarTheirBazThis PR has: