raminqaf opened a new pull request, #29835:
URL: https://github.com/apache/flink/pull/29835
## What is the purpose of the change
FLINK-40910 rejects VARIANT where a query needs equality or order. A
comparison of an expression with itself slipped through. With a VARIANT column
`v` holding `1` and `NULL`:
```sql
SELECT PARSE_JSON('1') = PARSE_JSON('1');
```
| Query | Before |
After |
|------------------------------------------------------|-----------------|----------|
| `SELECT PARSE_JSON('1') = PARSE_JSON('1')` | `TRUE` |
rejected |
| `SELECT v < v FROM t` | `FALSE`, `NULL` |
rejected |
| `SELECT v IN (v) FROM t` | `TRUE`, `NULL` |
rejected |
| `SELECT NULLIF(v, v) FROM t` | `NULL`, `NULL` |
rejected |
| `SELECT COUNT(*) FROM t a JOIN t b ON a.v = a.v` | `2` |
rejected |
| `SELECT COUNT(*) FROM t WHERE v = v` | rejected |
rejected |
Each now fails with the message that `PARSE_JSON('1') = PARSE_JSON('2')`
already got: `An expression of type VARIANT cannot be used in a comparison with
'=', because the type has no equality. Cast the value to a comparable type
first.`
`ComparableTypeValidator` runs on the logical plan. `SqlToRelConverter`
builds projections and join conditions with `RelBuilder`, which simplifies them
before that. `RexSimplify` rewrites a deterministic `x = x` to `NULL OR x IS
NOT NULL`, and `x <> x`, `x < x` and `x > x` to `NULL AND x IS NULL`:
```
EXPLAIN SELECT v = v, v <> v FROM t
LogicalProject(EXPR$0=[OR(null, IS NOT NULL($0))], EXPR$1=[AND(null, IS
NULL($0))])
```
WHERE was not affected, because the converter builds the filter without
`RelBuilder`. The Table API was not affected either, because the input type
strategies of `isEqual` and the other comparisons already reject VARIANT.
The fix checks comparisons on the validated SQL call, before the conversion.
No Calcite change is needed: Calcite only assumes that a type which passes
validation for `=` has a reflexive equality. I considered two alternatives.
Turning off `RelBuilder` simplification during the conversion would change the
logical plan of many existing plan tests. Giving VARIANT its own `RelDataType`
without comparability would touch every place that creates a VARIANT type, and
users would get Calcite's generic `Cannot apply '='` message.
Visible side effects:
- The message names the SQL operator, not the one the converter expands it
into: `'IS DISTINCT FROM'` instead of `'='`, `'IN'` and `'NOT IN'` instead of
`'='` and `'<>'`, and Calcite's `'BETWEEN ASYMMETRIC'` instead of `'>='`.
- A VARIANT comparison in a computed column, a watermark or a view now
fails at `CREATE` instead of at the first query.
- A comparison of VARIANT with another type in WHERE, such as `v = 1`, now
gets this message instead of Calcite's `Cannot apply '=' to arguments of type
'<VARIANT> = <INTEGER>'`, because the new check runs before Calcite's type
check there. In a SELECT list Calcite's message still comes first. Both reject
the query.
I plan to backport this to release-2.4, which ships FLINK-40910.
## Brief change log
- `FlinkCalciteSqlValidator.validateCall` passes each validated call to
the new `ComparableTypeValidator.validateComparison`.
- `validateComparison` checks the operands of `=`, `<>`, `<`, `<=`, `>`,
`>=`, `IS [NOT] DISTINCT FROM`, `[NOT] BETWEEN` and `[NOT] IN`, including each
value of an `IN` list. It shares the type check and message with the plan
check. The plan check stays for keys, functions and conditions the converter
adds, such as `JOIN ... USING`.
## Verifying this change
This change added tests and can be verified as follows:
- `ComparableTypeValidatorTest` has a new `SELECT PARSE_JSON('1') =
PARSE_JSON('1')` case and `testRejectedComputedColumn`. Without the fix both
fail with `Expecting code to raise a throwable.`
- The existing `BETWEEN`, `IN` and `IS [NOT] DISTINCT FROM` cases in
`ComparableTypeValidatorTest` and `VariantSemanticTest` now expect the SQL
operator name, so they pin that the new check catches them.
- The watermark of table `w` in `ComparableTypeValidatorTest` used `v =
PARSE_JSON('1')`, which now fails at `CREATE TABLE`. It now uses
`ARRAY_CONTAINS(a, v)`, which only the plan check rejects, so the watermark
branch of the plan check stays covered.
- Locally green: the `plan`, `operations`, `calcite`, `expressions` and
`functions` test packages of `flink-table-planner`, 16,249 tests, restore tests
excluded.
## Does this pull request potentially affect one of the following parts:
- Dependencies (does it add or upgrade a dependency): no
- The public API, i.e., is any changed class annotated with
`@Public(Evolving)`: no
- The serializers: no
- The runtime per-record code paths (performance sensitive): no, the check
runs during validation
- Anything that affects deployment or recovery: JobManager (and its
components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
- The S3 file system connector: no
## Documentation
- Does this pull request introduce a new feature? no
- If yes, how is the feature documented? not applicable
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code 2.1.319 (Claude Opus 5.5)
--
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]
