Allow casted literal values in SQL functions accepting literals #15282

LakshSingla · 2023-10-30T22:50:14Z

Description

For SQL functions accepting literals, casting might be allowed or disallowed, depending on whether the OperandTypes.LITERAL checker is used. Using it disallows casted literals like - CAST(100 AS INTEGER.
While this was an acceptable limitation as there's no use of casting a literal that the user has entered, the issue arises due to the Calcite upgrade, where the new JDBC client can't allow wildcards without an explicit cast. Therefore for a function with a signature like FUNC(column, literal), we can't do the following in JDBC.

SELECT FUNC(column, ?) FROM foo

Instead, we'd have to work around it using

SELECT FUNC(column, CAST(? AS TYPE)) FROM foo

But the second query doesn't play nice if the FUNC uses OperandTypes.LITERAL. Therefore this PR allows cast in such functions by modifying the signature to not rely on OperandTypes.LITERAL.

This PR also marks OperandTypes.LITERAL in forbidden-apis so that the user cannot use the LITERAL checker (until JDBC behavior is reverted, if it would).
BasicOperandTypeChecker is removed and the DefaultOperandTypeChecker is the only checker around.

Release note

Functions that accept literals also allow casted literals. This shouldn't have an impact on the queries that the user writes. It enables the SQL functions to accept explicit cast, which is required with JDBC.

This PR has:

sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java

abhishekagarwal87 · 2023-10-31T09:21:51Z

...query/aggregation/datasketches/quantiles/sql/DoublesSketchListArgBaseOperatorConversion.java

@@ -144,7 +143,7 @@ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean throwOnFail

        // Verify that 'operand' is a literal number.
        if (!SqlUtil.isLiteral(operand)) {


doesn't this need to allow cast?

Good catch, it does. I modified the call to allow casts and added test for the same

abhishekagarwal87 · 2023-10-31T09:29:07Z

sql/src/main/java/org/apache/druid/sql/calcite/expression/DefaultOperandTypeChecker.java

+    for (int i = requiredOperands; i < operands.size(); i++) {
+      ret.append("]");
+    }


this doesn't seem right. is there a test case to verify signature when there are multiple required operands?

Hmm, git diff shows that I moved BasicOperandTypeChecker -> DefaultOperandTypeChecker. However DefaultOperandTypeChecker was already present in OperatorConversions that I pulled out.

I think it creates a signature like:

'functionName(requiredOperand1, requiredOperand2, [optionalOperand1, [optionalOperand2]])'

We append 'functionName(

Loop begins

For each operand, if it's not the first operand, we append ,

For each operand, if its an optional operand, we append [. At the end of the loop, we'd have open square brackets equal to the number of the optional operands

For each operand, we append the operand name

Loop ends

For the total number of optional operands, we append ] at the end, therefore closing them out.

We append )'

There's an APPROX_COUNT_DISTINCT_DS_HLL that takes 3 parameters, out of which the first one is required and the rest two are optional.

There's BLOOM_FILTER that takes 2 parameters and both are required

And then there's APPROX_QUANTILE_FIXED_BUCKETS which takes in 6 parameters out of which first 5 are required.

ah yes. Now I get it. thanks for explaining the flow 😄

somu-imply

Thanks for the changes. Looks good !

gianm

LGTM, although I did have a suggestion for simplifying TDIGEST_GENERATE_SKETCH as well.

gianm · 2023-10-31T16:12:44Z

...org/apache/druid/query/aggregation/tdigestsketch/sql/TDigestGenerateSketchSqlAggregator.java

-                  OperandTypes.sequence(SIGNATURE_WITH_COMPRESSION, OperandTypes.ANY, OperandTypes.LITERAL),
-                  OperandTypes.family(SqlTypeFamily.ANY, SqlTypeFamily.NUMERIC)
-              )
+              // Validation for signatures like 'TDIGEST_GENERATE_SKETCH(column, compression)'


There's no need to have the OperandTypes.or anymore. Just use the default checker with requiredOperandCount(1).

Resolved! Thanks for the review

…he#15282) Functions that accept literals also allow casted literals. This shouldn't have an impact on the queries that the user writes. It enables the SQL functions to accept explicit cast, which is required with JDBC.

…) (#15298) Functions that accept literals also allow casted literals. This shouldn't have an impact on the queries that the user writes. It enables the SQL functions to accept explicit cast, which is required with JDBC. Co-authored-by: Laksh Singla <lakshsingla@gmail.com>

…he#15282) Functions that accept literals also allow casted literals. This shouldn't have an impact on the queries that the user writes. It enables the SQL functions to accept explicit cast, which is required with JDBC.

init commit

5321307

github-actions bot added the Area - Querying label Oct 30, 2023

github-advanced-security bot found potential problems Oct 30, 2023

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/expression/OperatorConversions.java Fixed Show fixed Hide fixed

LakshSingla added this to the 28.0 milestone Oct 31, 2023

remove BasicOperandTypeChecker

4d1cdfa

abhishekagarwal87 reviewed Oct 31, 2023

View reviewed changes

add changes for DoublesSketchListArgBaseOperatorConversion

1979bcb

abhishekagarwal87 approved these changes Oct 31, 2023

View reviewed changes

somu-imply approved these changes Oct 31, 2023

View reviewed changes

gianm approved these changes Oct 31, 2023

View reviewed changes

review

6ef4e89

abhishekagarwal87 merged commit 2ea7177 into apache:master Nov 1, 2023
82 checks passed

LakshSingla mentioned this pull request Nov 2, 2023

Allow casted literal values in SQL functions accepting literals (Part 2) #15316

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow casted literal values in SQL functions accepting literals #15282

Allow casted literal values in SQL functions accepting literals #15282

LakshSingla commented Oct 30, 2023 •

edited

Loading

abhishekagarwal87 Oct 31, 2023

LakshSingla Oct 31, 2023

abhishekagarwal87 Oct 31, 2023

LakshSingla Oct 31, 2023 •

edited

Loading

LakshSingla Oct 31, 2023 •

edited

Loading

abhishekagarwal87 Oct 31, 2023

somu-imply left a comment

gianm left a comment

gianm Oct 31, 2023

LakshSingla Oct 31, 2023

		@@ -144,7 +143,7 @@ public boolean checkOperandTypes(SqlCallBinding callBinding, boolean throwOnFail

		// Verify that 'operand' is a literal number.
		if (!SqlUtil.isLiteral(operand)) {

Allow casted literal values in SQL functions accepting literals #15282

Allow casted literal values in SQL functions accepting literals #15282

Conversation

LakshSingla commented Oct 30, 2023 • edited Loading

Description

Release note

abhishekagarwal87 Oct 31, 2023

Choose a reason for hiding this comment

LakshSingla Oct 31, 2023

Choose a reason for hiding this comment

abhishekagarwal87 Oct 31, 2023

Choose a reason for hiding this comment

LakshSingla Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

LakshSingla Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

abhishekagarwal87 Oct 31, 2023

Choose a reason for hiding this comment

somu-imply left a comment

Choose a reason for hiding this comment

gianm left a comment

Choose a reason for hiding this comment

gianm Oct 31, 2023

Choose a reason for hiding this comment

LakshSingla Oct 31, 2023

Choose a reason for hiding this comment

LakshSingla commented Oct 30, 2023 •

edited

Loading

LakshSingla Oct 31, 2023 •

edited

Loading

LakshSingla Oct 31, 2023 •

edited

Loading