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.

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

Return err if wildcard is not expanded before type coercion #14130

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

xudong963
Copy link
Member

Rationale for this change

If users use the whole analyzer, they won't encounter the issue. But if only use the TypeCoercionRewriter(Yes, it's me), such as

  if let LogicalPlan::Union(union) = logical_plan  {
      logical_plan = TypeCoercionRewriter::coerce_union(union)?;
  }

and if there is the wildcard in the union, such as the test in the PR, I'll get the error https://github.com/apache/datafusion/blob/main/datafusion/expr/src/logical_plan/plan.rs#L2138.

What changes are included in this PR?

Make the error report early and clear, the current change will directly tell the user what they are missing.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the optimizer Optimizer rules label Jan 15, 2025
@xudong963 xudong963 requested a review from alamb January 15, 2025 01:26
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @xudong963 -- this makes sense to me

cc @jonahgao for your thoughts in case you would also like to review

@alamb
Copy link
Contributor

alamb commented Jan 15, 2025

I thin you can fix the CI test by using assert_contains! rather than checking the exact error message

let sql_to_rel = SqlToRel::new(&context_provider);
let logical_plan = sql_to_rel.sql_statement_to_plan(statements[0].clone())?;

if let LogicalPlan::Union(union) = logical_plan {
Copy link
Member

Choose a reason for hiding this comment

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

We need to ensure that logical_plan is definitely a union. This could happen if sql_statement_to_plan changes in the future, and then this test would become invalid. We can do it like this:

    let LogicalPlan::Union(union) = logical_plan else {
        panic!("Expected Union plan");
    };
    let err = TypeCoercionRewriter::coerce_union(union)
        .err()
        .unwrap()
        .to_string();
    assert_contains!(
        err,
        "Error during planning: Wildcard should be expanded before type coercion"
    );

Copy link
Member

@jonahgao jonahgao 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 @xudong963 . I have a small suggestion about the test, and we need to fix the clippy error in this PR.

@alamb
Copy link
Contributor

alamb commented Jan 15, 2025

I merged this PR up from main and merged a clippy fix

@xudong963
Copy link
Member Author

thanks @alamb @jonahgao —ill merge it

@xudong963 xudong963 merged commit d42b994 into apache:main Jan 15, 2025
25 checks passed
@xudong963 xudong963 deleted the report_error_early branch January 15, 2025 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants