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

Remove unused binary_array_op_scalar! in binary.rs #2512

Merged
merged 1 commit into from
May 20, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented May 11, 2022

Draft until #2510 is merged

Which issue does this PR close?

re #1179 and #2482 .

Rationale for this change

(the real reason for this is that I got nerd-sniped trying to clean up expression evaluation )

  1. The binary_array_op_scalar! doesn't handle null constants (see fix NULL <op> column evaluation, tests for same #2510)
  2. It is duplicative of binary_array_op_dyn_scalar!

What changes are included in this PR?

  1. Remove binary_array_op_dyn_scalar!
  2. Remove code that then becomes unused without that macro 🎉

Are there any user-facing changes?

No, all internal changes

@github-actions github-actions bot added the datafusion Changes in the datafusion crate label May 11, 2022
@@ -1142,6 +1079,20 @@ macro_rules! binary_array_op_dyn_scalar {
}}
}

/// Compares the array with the scalar value for equality, sometimes
/// used in other kernels
pub(crate) fn array_eq_scalar(lhs: &dyn Array, rhs: &ScalarValue) -> Result<ArrayRef> {
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 made a function to call binary_array_op_dyn_scalar! from null_if rather than having it call the macro directly to clean that code up.


let array = primitive_bool_array_op!(lhs, *cond_array, nullif)?;

Ok(ColumnarValue::Array(array))
}
(ColumnarValue::Array(lhs), ColumnarValue::Array(rhs)) => {
// Get args0 == args1 evaluated and produce a boolean array
let cond_array = binary_array_op!(lhs, rhs, eq)?;
let cond_array = eq_dyn(lhs, rhs)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hooray for eq_dyn thanks again @jimexist

@alamb alamb marked this pull request as ready for review May 18, 2022 13:19
@alamb alamb changed the title Remove remaining uses of binary_array_op_scalar! in binary.rs Remove unused binary_array_op_scalar! in binary.rs May 18, 2022
@alamb alamb requested review from jimexist and andygrove May 18, 2022 13:52
@alamb
Copy link
Contributor Author

alamb commented May 18, 2022

cc @WinkerDu @yjshen

@andygrove andygrove merged commit 7b95dcb into apache:master May 20, 2022
ovr pushed a commit to cube-js/arrow-datafusion that referenced this pull request Aug 15, 2022
@alamb alamb deleted the alamb/clean_up_binary branch August 8, 2023 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
datafusion Changes in the datafusion crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants