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

Chore: explicitly list out all Expr types in TypeCoercionRewriter::mutate #9038

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

guojidan
Copy link
Contributor

Which issue does this PR close?

Closes #8990 .

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the optimizer Optimizer rules label Jan 29, 2024
@guojidan
Copy link
Contributor Author

guojidan commented Jan 29, 2024

indexmap::IndexSet crate will deprecate remove method at 2.2.0 version, but we use 2.0.0, why test cases fail? 😕

and remove methed just a wrap of swap_remove method, Can we directly use swap_remove? 🤔

#9037 Failed due to the same reason

@alamb
Copy link
Contributor

alamb commented Jan 29, 2024

The CI failure was fixed in #9034

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.

Thank you @guojidan -- this is a nice improvement

| Expr::SimilarTo(_)
| Expr::IsNotNull(_)
| Expr::IsNull(_)
| Expr::Negative(_)
Copy link
Contributor

@alamb alamb Jan 29, 2024

Choose a reason for hiding this comment

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

🤔 looks like there are some other bugs here;

❯ select -'100';
Internal error: Can't create negative physical expr for (- 'Literal { value: Utf8("100") }'), the type of child expr is Utf8, not signed numeric.
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
❯ select -'100'::int;
+-----------------+
| (- Utf8("100")) |
+-----------------+
| -100            |
+-----------------+

I will file another ticket -- #9060

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 @guojidan -- this is awesome. Even better it helped find another bug 🎉

@alamb alamb merged commit 851bc7d into apache:main Jan 29, 2024
21 of 22 checks passed
@guojidan guojidan deleted the match branch January 30, 2024 00:48
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.

explicitly list out all Expr types in TypeCoercionRewriter::mutate
2 participants