Skip to content

[CALCITE-7457] VALUES and SELECT produce different validation results for the same expression #4858

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.

Sign up for GitHub

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

Open
dssysolyatin wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[CALCITE-7457] VALUES and SELECT produce different validation results for the same expression #4858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions core/src/main/java/org/apache/calcite/sql/SqlValuesOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
*/
package org.apache.calcite.sql;

import org.apache.calcite.sql.validate.SqlValidator;
import org.apache.calcite.sql.validate.SqlValidatorScope;

/**
* The <code>VALUES</code> operator.
*/
Expand All @@ -28,6 +31,11 @@ public SqlValuesOperator() {

//~ Methods ----------------------------------------------------------------

@Override public void validateCall(SqlCall call, SqlValidator validator,
SqlValidatorScope scope, SqlValidatorScope operandScope) {
validator.validateQuery(call, scope, validator.getUnknownType());
}

@Override public void unparse(
SqlWriter writer,
SqlCall call,
Expand Down
3 changes: 3 additions & 0 deletions core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2306,6 +2306,9 @@ protected void inferUnknownTypes(
scope = getMeasureScope(((SelectScope) scope).getNode());
}
inferUnknownTypes(inferredType, scope, ((SqlCall) node).operand(0));
} else if (node.isA(SqlKind.QUERY)) {
// Do not descend into subqueries. Each query (SELECT, VALUES,
// etc.) calls inferUnknownTypes during its own validation.
} else if (node instanceof SqlCall) {
final SqlCall call = (SqlCall) node;
final SqlOperandTypeInference operandTypeInference =
Expand Down
76 changes: 65 additions & 11 deletions core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1322,13 +1322,20 @@ void testLikeAndSimilarFails() {
expr("values 1.0 + ^NULL^")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
// SELECT produces DECIMAL(3, 1) because inferUnknownTypes infers
// NULL as DECIMAL(2, 1) via FIRST_KNOWN, and DECIMAL(2,1) + DECIMAL(2,1)
// = DECIMAL(3, 1)
sql("select 1.0 + NULL from (values (0)) as t(x)")
.columnType("DECIMAL(3, 1)");
expr("values 1.0 + NULL")
.columnType("DECIMAL(2, 1)");
.columnType("DECIMAL(3, 1)");
Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should this inferred type be different?
Something is off.

Copy link
Copy Markdown
Contributor Author

@dssysolyatin dssysolyatin Apr 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explained above, in the SELECT section, why this happens. I'm not saying it's the correct behavior and I would expect that 1.0 + NULL = NULL with type DECIMAL(2,1) (common not null values type) in this case, but currently it works like that

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you say SELECT 1.0 + NULL already was DECIMAL(3,1), but when used in VALUES the result was different?

Copy link
Copy Markdown
Contributor Author

@dssysolyatin dssysolyatin Apr 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if you open task description - https://issues.apache.org/jira/browse/CALCITE-7457. You can see test case for main branch

expr("1.0 + ^NULL^")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
sql("select 1.0 + NULL from (values (0)) as t(x)")
.columnType("DECIMAL(3, 1)");
expr("1.0 + NULL")
.columnType("DECIMAL(2, 1)");
.columnType("DECIMAL(3, 1)");

// FIXME: SQL:2003 does not allow raw NULL in IN clause
expr("1 in (1, null, 2)").ok();
Expand Down Expand Up @@ -1581,9 +1588,14 @@ void testLikeAndSimilarFails() {
expr("LOCALTIME").ok(); // fix sqlcontext later.
wholeExpr("LOCALTIME(1+2)")
.fails("Argument to function 'LOCALTIME' must be a literal");
wholeExpr("LOCALTIME(NULL)")
// With type coercion disabled, inferUnknownTypes rejects NULL before
// LOCALTIME can validate. SELECT produces the same error.
sql("select LOCALTIME(^NULL^) from (values (0)) as t(x)")
.withTypeCoercion(false)
.fails("Argument to function 'LOCALTIME' must not be NULL");
.fails("(?s).*Illegal use of .NULL.*");
expr("LOCALTIME(^NULL^)")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
wholeExpr("LOCALTIME(NULL)")
.fails("Argument to function 'LOCALTIME' must not be NULL");
wholeExpr("LOCALTIME(CAST(NULL AS INTEGER))")
Expand Down Expand Up @@ -7924,6 +7936,35 @@ void testGroupExpressionEquivalenceParams() {
.ok();
}

/**
* Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-7457">[CALCITE-7457]
* VALUES and SELECT produce different validation results for the same expression</a>.
*/
@Test void testSelectVsValuesValidation() {
sql("select 1.0 + NULL from (values (0)) as t(x)")
.columnType("DECIMAL(3, 1)");
expr("1.0 + NULL")
.columnType("DECIMAL(3, 1)");

sql("select 1 + ? from (values (0)) as t(x)").ok();
expr("1 + ?").ok();

sql("select 1 + ^NULL^ from (values (0)) as t(x)")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
expr("1 + ^NULL^")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");

sql("select LOCALTIME(^NULL^) from (values (0)) as t(x)")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
expr("LOCALTIME(^NULL^)")
.withTypeCoercion(false)
.fails("(?s).*Illegal use of .NULL.*");
}

@Test void testPercentileFunctionsBigQuery() {
final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);
final String sql = "select\n"
Expand Down Expand Up @@ -8104,15 +8145,28 @@ void testGroupExpressionEquivalenceParams() {
+ " overlaps (date '1-2-3', date '1-2-3')^\n"
+ "or false")
.fails("(?s).*Cannot apply 'OVERLAPS' to arguments of type .*");
// row with 3 arguments as right argument to overlaps
// row with 3 arguments as right argument to overlaps.
// validateValues checks ROW structure before OVERLAPS validates,
// producing "Unequal number of entries in ROW expressions".
// SELECT produces the same error.
sql("select true or"
+ " (date '1-2-3', date '1-2-3')"
+ " overlaps ^(date '1-2-3', date '1-2-3', date '1-2-3')^"
+ " or false from (values (0)) as t(x)")
.fails("(?s).*Unequal number of entries in ROW expressions.*");
expr("true\n"
+ "or ^(date '1-2-3', date '1-2-3')\n"
+ " overlaps (date '1-2-3', date '1-2-3', date '1-2-3')^\n"
+ "or (date '1-2-3', date '1-2-3')\n"
+ " overlaps ^(date '1-2-3', date '1-2-3', date '1-2-3')^\n"
+ "or false")
.fails("(?s).*Cannot apply 'OVERLAPS' to arguments of type .*");
expr("^period (date '1-2-3', date '1-2-3')\n"
+ " overlaps (date '1-2-3', date '1-2-3', date '1-2-3')^")
.fails("(?s).*Cannot apply 'OVERLAPS' to arguments of type .*");
.fails("(?s).*Unequal number of entries in ROW expressions.*");
Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression, there is not ROW expression in the original program. The user will have difficulties finding the location of this error.

Copy link
Copy Markdown
Contributor Author

@dssysolyatin dssysolyatin Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that the original query does not contain a ROW. However, during parsing, the OVERLAPS arguments are wrapped inside a ROW prior to any validation

Currently, VALUES follows the proper execution order:
a. Operand inference
b. Operand validation

Operand inference throws an exception, but it probably should not. It should probably simply leave the types unfilled. Operand validation would then fail with something like

Cannot apply 'OVERLAPS' to arguments of type '<RECORDTYPE(DATE EXPR$0, DATE EXPR$1)> OVERLAPS <RECORDTYPE(DATE EXPR$0, DATE EXPR$1, DATE EXPR$2)>'. Supported form(s): '(<DT>, <DT>) OVERLAPS (<DT>, <DT>)'
'(<DT>, <DT>) OVERLAPS (<DT>, <INTERVAL>)'
'(<DT>, <INTERVAL>) OVERLAPS (<DT>, <DT>)'
'(<DT>, <INTERVAL>) OVERLAPS (<DT>, <INTERVAL>)'

Regarding the regression, I'm not sure it is regression, since SELECT already had the same behavior. Also SELECT * FROM VALUES()/INSERT/UPDATE/DELETE behave the same as they call validateValues which first:

  1. Does operand inference
  2. And then does operand validation

Ironically, VALUES expression (when VALUES is top level node) were the exception.

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know why no checks were run in CI?
I would expect this error using ROW to appear in many more cases.

Copy link
Copy Markdown
Contributor Author

@dssysolyatin dssysolyatin Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is because I add LGMT-will-merge-soon after first approve, it run CI one more time but with specific CI tests

Copy link
Copy Markdown
Contributor

@mihaibudiu mihaibudiu Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please run all the tests to check that there aren't other tests affected by the ROW message?

Copy link
Copy Markdown
Contributor Author

@dssysolyatin dssysolyatin Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those all ran before I added the LGTM-will-merge-soon label. Anyway, I made a small tweak based on #4858 (comment). Re-running now

// SELECT produces the same error for mismatched ROW sizes
sql("select period (date '1-2-3', date '1-2-3')"
+ " overlaps ^(date '1-2-3', date '1-2-3', date '1-2-3')^"
+ " from (values (0)) as t(x)")
.fails("(?s).*Unequal number of entries in ROW expressions.*");
expr("period (date '1-2-3', date '1-2-3')\n"
+ " overlaps ^(date '1-2-3', date '1-2-3', date '1-2-3')^")
.fails("(?s).*Unequal number of entries in ROW expressions.*");
expr("true\n"
+ "or ^(1, 2) overlaps (2, 3)^\n"
+ "or false")
Expand Down
Loading