nullable Expr being constant fold to value can cause schema change and internal error

[eejbyfeldt]

Describe the bug

Some Exprs in DataFusion (mostly ScalarFunctions) are defined nullable but when being constant evaluated end up producing a (non-null) value. Since the replacement Expr::Literal is no longer nullable, this can in some cases lead to a schema change and trigger physical/logical schema mismatch check added in #11989

To Reproduce

Running the following query in datafusion cli

> SELECT combined FROM (
  SELECT concat('A', 'B') AS combined
  UNION ALL
  SELECT concat('A', 'B') AS combined
) GROUP BY combined;
Internal error: Physical input schema should be the same as the one converted from logical input schema..
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

Expected behavior

The query should produce a result without errors.

Additional context

No response

[findepi]

Good catch.
To me this suggests that the schema check is too strict.
It should allow fulfilling nulllable contract with non-null data (but not the other way around).

[comphead]

Agree, we touched this a little bit with @alamb here #13065 (comment) to have the same solution like it was before DF 42 but its still pending.

Probably what we can do is implement Eq with full check and with parameter to bypass nullability and metadata check, the parameter will be disabled by default providing full eq check but gives an opportunity to disable it with minimum risks.

WDYT guys?

[findepi]

Would that mean that constant folding (like this issue) requires the relaxed comparison parameter to be set?
it would be the default behavior then, right?

Probably what we can do is implement Eq with full check

on the code level, i agree that Eq should do the full check. Value-based classes should be compared by value in general.
For logical/physical schema comparison and nulls, we can choose not to call Eq.

[comphead]

We can rewrite Eq to check only what we need just make syntax clean, to preserve = for comparison. Another option is to create our own comparison method. The problem with own method is the developer/user might not use and use = sign

[findepi]

The problem with own method is the developer/user might not use and use = sign

good point. then let's not support Eq trait and things will be clear.

[alamb]

Here is existing code to compare only names / types of schemas (not nullabilty / metadata too):
https://docs.rs/datafusion/latest/datafusion/common/trait.SchemaExt.html#tymethod.logically_equivalent_names_and_types

[findepi]

My current thinking is that we should compare all: names, types, metadata¹ and nullability with the only exception being that non-null schema can be used to satisfy nullable requirements. Ie what we need might be asymmetrical "is satisfied by" relation rather than eq / partialEq symmetrical relation.

Footnotes

1. From "is satisfied by" perspective, ignoring metadata is fine, but my guess is that the metadata is what gets output with query results, so it should be preserved. query result is the highest def of the semantics, and if we consider metadata part of the result, then it needs to match. ↩

[jayzhan211]

You're saying that if the schema changes from nullable to non-null, we should consider this an acceptable change, correct? That makes sense—the idea is that the transformed schema can be more strict, but not more flexible.

[findepi]

You're saying that if the schema changes from nullable to non-null, we should consider this an acceptable change, correct?

yes, exactly

[tv42]

For others stumbling on this, you can disable this check with

        let options = session_config.options_mut();
        options.execution.skip_physical_aggregate_schema_check = true;

[tv42]

And a debugging hint for others, because this message might not imply a Datafusion bug in all cases:

I refactored my TableProvider and forgot to apply the given Projection to the schema it returns. This caused "column index 1" to not mean what it was supposed to mean.

This same bug on my part also caused

query failed: Internal error: Physical input schema should be the same as the one converted from logical input schema. Differences:
- Different number of fields: (physical) 2 vs (logical) 0.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker

Internal error: Input field name a does not match with the projection expression b.
This was likely caused by a bug in DataFusion's code and we would welcome that you file an bug report in our issue tracker
[SQL] UPDATE kanto.myschema.mytable SET a=41 WHERE b=7;

test panicked: index out of bounds: the len is 1 but the index is 1

(backtrace said it was a .column(idx) deep in the guts of Datafusion, async meant backtrace didn't refer to my code at all)

If anyone wants to come up with more pinpoint debug assertions to catch buggy TableProviders, that'd be neat.

[findepi]

maybe we just change the check code - #14519