-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow casted literal values in SQL functions accepting literals (Part 2) #15316
Allow casted literal values in SQL functions accepting literals (Part 2) #15316
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: What are the implications of this during calcite upgrade ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or better yet move them to apache calcite ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
MyFoo
OurBar
TheirBaz
This PR has: