pingcap / pingcap/tidb

planner: ExtractFD misuses null-reject proofs for not-null inference

Open
#68,026 2 comments 0 reactions 0 assignees View on GitHub
affects-7.1 affects-7.5 affects-8.1 affects-8.5 affects-9.0 severity/major sig/planner type/bug
Dominant language
Go
Stars
40.5k
Forks
6.2k
PR merge metrics
PR metrics pending

Description

## Bug Report

### 1. Minimal reproduce step (Required)

`ExtractFD` currently uses `IsNullRejected` in several places where the caller actually needs a different not-null proof. There are at least two distinct caller semantics:

1. Filter-derived column not-null inference: whether `condition = TRUE` implies a specific column is non-NULL.
2. Projection/Aggregation scalar output nullability: whether a scalar expression result is guaranteed to be non-NULL.

`IsNullRejected(ctx, schema, predicate)` proves a different property: after every column in `schema` is replaced with SQL NULL, `predicate` cannot evaluate to TRUE. That is correct for outer-join simplification against the null-producing side, but it is not directly a general not-null proof.

#### Case 1: filter not-null inference incorrectly uses whole-schema null-reject

Reproduction:

```sql
set @@session.tidb_enable_new_only_full_group_by_check = 'on';

create table x3(
a int not null primary key,
b int not null,
c int default null,
d int not null,
unique key I_b_c (b,c)
);

select a from x3 where c = 1 or d = 1 group by b,c;
```

The planner currently accepts the query. A local planner FD check on current master shows that `ExtractFD` upgrades the nullable unique key `(b,c)` into a strict FD:

```text
Plan: DataScan(x3)->Aggr(firstrow(test.x3.a))->Projection
FD: {(1)-->(2-5), (2,3)~~>(1,4,5)}
>>> {(1)-->(2-5), (2,3)-->(1,4,5)}
>>> {(1)-->(2,3), (2,3)-->(1)}
```

This comes from `ExtractNotNullFromConds`: it calls `IsNullRejected(p.SCtx(), p.Schema(), condition)` once for the whole schema and then marks every column referenced by the condition as not-null.

However, whole-schema null-reject does not imply each referenced column must be non-NULL when the predicate is TRUE. For `c = 1 OR d = 1`, replacing both `c` and `d` with NULL is null-rejected, but `c` itself is not required to be non-NULL because the predicate is TRUE for rows where `c IS NULL AND d = 1`.

Concrete data shape:

```sql
insert into x3 values (1, 10, null, 1), (2, 10, null, 1);
```

Both rows satisfy `c = 1 OR d = 1`, and `unique(b,c)` permits multiple rows with `c IS NULL`. Therefore `(b,c)` does not functionally determine `a` after filtering.

For this caller, the intended property should be checked per column, for example by proving null-rejection against a single-column schema for each referenced column.

#### Case 2: Projection/Aggregation scalar output nullability uses predicate null-reject

`LogicalProjection.ExtractFD` and `LogicalAggregation.ExtractFD` also call `IsNullRejected` to decide whether a scalar expression's output column should be marked not-null:

```go
notnull := util.IsNullRejected(p.SCtx(), p.Schema(), x)
if notnull || determinants.SubsetOf(fds.NotNullCols) {
notnullColsUniqueIDs.Insert(scalarUniqueID)
}
```

and similarly in `LogicalAggregation.ExtractFD`.

This is not the same property. For example, as a filter predicate, `a = 1` is null-rejected with respect to `a`: if `a` is NULL, the predicate cannot be TRUE. But as a projected scalar expression, `a = 1` can evaluate to NULL when `a` is NULL, so the output column is not guaranteed to be non-NULL.

Those callers need an expression-result nullability proof instead of a predicate null-reject proof. A conservative fix would avoid marking scalar outputs not-null from `IsNullRejected` and only use a helper that proves the expression result itself cannot be NULL.

### 2. What did you expect to see? (Required)

For the filter-derived FD case, the query should be rejected by `ONLY_FULL_GROUP_BY`, because `a` is not functionally dependent on `GROUP BY b,c` after the filter.

For Projection/Aggregation, scalar expression output columns should only be marked not-null when the expression result itself is guaranteed to be non-NULL. Predicate null-rejection should not be used as a substitute for output nullability.

### 3. What did you see instead (Required)

The filter case is accepted. `ExtractFD` treats `(b,c)` as a strict FD determinant and allows `a` to be selected with `GROUP BY b,c`.

Projection/Aggregation can also mark scalar expression outputs as not-null based on predicate null-rejection semantics, which is stronger than what those callers can safely conclude.

### 4. What is your TiDB version? (Required)

Observed on current master at:

```text
3c8816c0143f80a93a98c8b4f9d13a3f898cb0dc
```

Suggested labels: `type/bug`, `sig/planner`, `severity/major`, `affects-9.0`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.