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

minor: port some expr tests to sqllogictests, improve error message #5968

Merged
merged 2 commits into from
Apr 13, 2023
Merged

minor: port some expr tests to sqllogictests, improve error message #5968

merged 2 commits into from
Apr 13, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Apr 11, 2023

Which issue does this PR close?

Part of #4495

Rationale for this change

  1. Suggested by @comphead as part of feat: Implement the bitwise_not in NotExpr #5902 (comment)
  2. https://github.com/apache/arrow-datafusion/tree/main/datafusion/core/tests/sqllogictests#sqllogictests
  3. I would like to evaluate test coverage in one place

What changes are included in this PR?

  1. Port some tests over
  2. Update a message from internal to plan

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Apr 11, 2023
alamb
alamb commented Apr 11, 2023
View reviewed changes
datafusion/physical-expr/src/expressions/not.rs
@@ -77,8 +77,8 @@ impl PhysicalExpr for NotExpr {
DataType::Int32 => Ok(DataType::Int32),
DataType::Int64 => Ok(DataType::Int64),
DataType::Null => Ok(DataType::Null),
_ => Err(DataFusionError::Internal(format!(
"Can't NOT or BITWISE_NOT datatype: '{:?}'",
_ => Err(DataFusionError::Plan(format!(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be hit with a bad user query, so it shouldn't be an Internal error

@alamb alamb mentioned this pull request Apr 11, 2023
Merged
@alamb alamb requested a review from xudong963 April 12, 2023 20:36
xudong963
xudong963 approved these changes Apr 13, 2023
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment

Choose a reason for hiding this comment

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

LGTM

@xudong963 xudong963 merged commit 2c57310 into apache:main Apr 13, 2023
korowa pushed a commit to korowa/arrow-datafusion that referenced this pull request Apr 13, 2023
@alamb @korowa
minor: port some expr tests to sqllogictests, improve error message (a…
9a4a5b3 
…pache#5968)

* minor: port some expr tests to sqllogictests, improve error message

* touchups
@alamb alamb deleted the alamb/move_expr_tests branch April 13, 2023 17:59
@alamb
Copy link
Contributor Author

alamb commented Apr 13, 2023 •
edited
Loading

Thanks for the assist @xudong963

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

@xudong963 xudong963 xudong963 approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alamb @xudong963