Skip to content
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

Jump to bottom

Add config option skip_physical_aggregate_schema_check #13176

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Add config option skip_physical_aggregate_schema_check #13176

merged 3 commits into from
Oct 30, 2024

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 29, 2024 •
edited
Loading

Which issue does this PR close?

Closes #13065

Rationale for this change

Some plans that used to run in DataFusion 41.0.0 started erroring in DataFusion 42.0.0 due to a new check that was added.

The delta-rs 42.0.0 upgrade has it this delta-io/delta-rs#2886 (comment), and we hit the same thing in InfluxDB IOx as well.

To help unblock upgrades, I would like to permit users to optionally disable this check.

This config flag is meant to be temporary while we fix all the underlying bugs.

Background: a new check for exact schema equality added in #11989 (released in DataFusion 42.0.0). This has found quite a few bugs where the schema doesn't quite match due to nullability or metadata mismatch -- see the list on #12733. There is at least one more bug @wiedld and I are tracking down.

After upgrading to DataFusion 42 some of these plans now error (due to bugs in DataFusion, see list on #12733)

What changes are included in this PR?

Add a skip_physical_aggregate_schema_check option to disable this check so downstream users can workaround any issues they hit

Are these changes tested?

The doc is tested by CI. Since this a workaround for bugs

Are there any user-facing changes?

A new config option

@github-actions github-actions bot added documentation Improvements or additions to documentation core Core DataFusion crate common Related to common crate labels Oct 29, 2024
@alamb alamb mentioned this pull request Oct 29, 2024
Closed
@alamb alamb changed the title Add option to skip physical aggregate check Add config option skip_physical_aggregate_schema_check Oct 29, 2024
@alamb
Copy link
Contributor Author

alamb commented Oct 29, 2024

I plan to backport this option to the 42 branch as well defaulted to true to unblock the delta-rs upgrade

comphead
comphead approved these changes Oct 29, 2024
View reviewed changes
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @alamb

@alamb
Copy link
Contributor Author

alamb commented Oct 29, 2024

See additional context here: #13065 (comment)

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 29, 2024
comphead
comphead reviewed Oct 29, 2024
View reviewed changes
datafusion/common/src/config.rs
/// When set to true, skips verifying that the schema produced by
/// planning the input of `LogicalPlan::Aggregate` exactly matches the
/// schema of the input plan.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if blank lines can break the formatting and tests eventually failed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the reason the tests failed before is that I forgot to update the information_schema.slt test

@alamb alamb merged commit 63e8e6a into apache:main Oct 30, 2024
26 checks passed
@alamb
Copy link
Contributor Author

alamb commented Oct 30, 2024

Thanks again @comphead and @jayzhan211

@alamb alamb deleted the alamb/config_option branch October 30, 2024 16:42
alamb added a commit to alamb/datafusion that referenced this pull request Oct 30, 2024
@alamb
Add config option skip_physical_aggregate_schema_check (apache#13176)
54fdc5d 
* Add option to skip physical aggregate check

* tweak wording

* update test
@alamb alamb mentioned this pull request Oct 30, 2024
Merged
@alamb
Copy link
Contributor Author

alamb commented Oct 30, 2024

PR to backport #13189

alamb added a commit that referenced this pull request Oct 30, 2024
@alamb
Backport config option skip_physical_aggregate_schema_check #13176 …
e7877e2 
…to 42 (#13189)

* Add config option `skip_physical_aggregate_schema_check ` (#13176)

* Add option to skip physical aggregate check

* tweak wording

* update test

* Change default value
@alamb alamb mentioned this pull request Oct 30, 2024
Open
8 tasks
@findepi
Copy link
Member

findepi commented Oct 31, 2024

Looking at #13190
It seems that schema check is too strict (It should allow fulfilling nulllable contract with non-null data, but not the other way around).

If we relax the check around this, how many problems would remain to be addressed?

@alamb
Copy link
Contributor Author

alamb commented Nov 1, 2024

Looking at #13190 It seems that schema check is too strict (It should allow fulfilling nulllable contract with non-null data, but not the other way around).

If we relax the check around this, how many problems would remain to be addressed?

It seems to me the check is finding real things (where the schema of the ExecutionPlan doesn't match the schema of the LogicalPlan from when it was created)

However, it also seems to me like maybe the discrepancy isn't a huge deal (so maybe it shouldn't be an error 🤔 )

@findepi
Copy link
Member

findepi commented Nov 1, 2024

Real things like type mismatch, or real things like stricter non-null guarantees (which may be considered an optimization, not an error)?

@alamb
Copy link
Contributor Author

alamb commented Nov 1, 2024

Real things like type mismatch, or real things like stricter non-null guarantees (which may be considered an optimization, not an error)?

What I know of was related to non-null guarantees as well as user defined metadata (on the Schema and the Field)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@comphead comphead comphead approved these changes

@jayzhan211 jayzhan211 jayzhan211 approved these changes

Assignees
No one assigned
Labels
common Related to common crate core Core DataFusion crate documentation Improvements or additions to documentation sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to disable Physical input schema should be the same as the one converted from logical schema error
4 participants
@alamb @findepi @comphead @jayzhan211