Skip to content

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

@alamb alamb force-pushed the alamb/clean_up_binary branch from cf40c4a to 22efa7c Compare May 11, 2022 13:27

/// 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.

(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 force-pushed the alamb/clean_up_binary branch from 22efa7c to 18683d6 Compare May 18, 2022 13:15
@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 andygrove and jimexist 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants