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

feat: impl subtract for primitive_scalar #1592

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from
Open

feat: impl subtract for primitive_scalar #1592

wants to merge 6 commits into from
+235 −86

Conversation

doki23
Copy link
Contributor

@doki23 doki23 commented Dec 6, 2024

part of #1591

@gatesn
Copy link
Contributor

gatesn commented Dec 10, 2024

Hey @doki23, are these PRs ready for review? Mark them non-draft when ready and we can take a look

@doki23
Copy link
Contributor Author

doki23 commented Dec 11, 2024

Hey @doki23, are these PRs ready for review? Mark them non-draft when ready and we can take a look

Yes, I had a travel last days:), I'll change its state, PTAL

@doki23 doki23 marked this pull request as ready for review December 11, 2024 01:01
@doki23
Copy link
Contributor Author

doki23 commented Dec 11, 2024

Maybe we could impl std::ops::Sub for it.

@doki23 doki23 force-pushed the primitve_scalar_subtract branch from db06957 to 01b8b6c Compare December 11, 2024 03:22
@doki23 doki23 force-pushed the primitve_scalar_subtract branch from fe28b95 to 8164019 Compare December 11, 2024 03:51
@gatesn
Copy link
Contributor

gatesn commented Dec 12, 2024

Can you explain a bit about how this is going to be used? I'm wondering whether these operations should be implemented on Scalar, PrimitiveScalar, or PValue.

@doki23
Copy link
Contributor Author

doki23 commented Dec 12, 2024

Can you explain a bit about how this is going to be used? I'm wondering whether these operations should be implemented on Scalar, PrimitiveScalar, or PValue.

If we implement fn subtract_scalar(&self, array: &ArrayData, to_subtract: &Scalar) -> VortexResult<ArrayData> for ConstantEncoding, we can get the result scalar by array.scalar().as_primitive() - to_subtract.as_primitive().

@doki23
Copy link
Contributor Author

doki23 commented Dec 17, 2024

Hey @gatesn , please take a look and let's move it forward.

gatesn
gatesn reviewed Dec 17, 2024
View reviewed changes
vortex-scalar/src/primitive.rs Outdated
@@ -103,6 +105,27 @@ impl<'a> PrimitiveScalar<'a> {
}?)),
}
}

pub fn subtract(&self, other: &PrimitiveScalar) -> VortexResult<Self> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you call this checked_sub(&self, other: &PrimitiveScalar) -> VortexResult<Option<Self>> and then use num-traits CheckedSub to ensure the subtraction doesn't overflow. Otherwise this function can panic.

Otherwise looks good to me!

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'm not sure the float checked_sub is correct, please review.

gatesn
gatesn reviewed Dec 17, 2024
View reviewed changes
vortex-scalar/src/primitive.rs Outdated
}
})
} else {
match_each_float_ptype!(self.ptype, |$T| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just do regular subtraction for floats. There's not exactly overflow behavior

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 think it's ok now

@danking danking self-assigned this Dec 17, 2024
@danking danking self-requested a review December 17, 2024 19:50
@lwwmanning lwwmanning changed the title impl subtract for primitive_scalar feat: impl subtract for primitive_scalar Dec 17, 2024
@gatesn
Copy link
Contributor

gatesn commented Dec 18, 2024

Sorry to be a pain again! @danking merged a change to the macro to let you implement different code blocks for ints and floats, e.g. https://github.com/spiraldb/vortex/pull/1640/files#diff-8f1cbe207a4076299ad7913ac7d7d0db5fd2e99d1a689f984fcedfd1b91208faR308-R329

And also actually that PR added arithmetic operations to arrays. Not sure if that's sufficient for you?

@danking
Copy link
Member

danking commented Dec 18, 2024

I merged and switched to the new macro for you @doki23 .

My general inclination is to use the (just added; sorry, we were working in parallel!) checked_numeric_operator method I just added, but there's a key difference between my implementation and yours. You avoid allocating or cloning a DType for each operation.

In the use cases I had (implementing BinaryNumericFn for ConstantArray and SparseArray), I always needed a Scalar anyway. But I think your approach seems better. Let me rework checked_numeric_operator to return a PrimitiveScalar<'a>. Then I'll push that to this branch and use it as the implementation of checked_sub. There's a bit more overhead (a match on the operator), but I think this will strike a good balance between maintainability while still avoiding allocating the DType if you don't want it.

I'll plan to have this pushed later today.

@danking
1. checked_numeric_operator returns a PrimitiveScalar (avoiding a D…
530c2c0 
…Type allocation)

2. `checked_sub` delegates to `checked_numeric_operator`

As a consequence of (2), `checked_sub` now returns an `None` when there is an underflow/overflow
rather than returning a Null PrimitiveScalar (the return type is now
`VortexResult<Option<PrimitiveScalar<'a>>>`).

Moreover, `std::ops::Sub` now returns an error on underflow/overflow rather than a Null
PrimitiveScalar.
@danking
Copy link
Member

danking commented Dec 19, 2024

Okay, here's what I did:

  1. checked_numeric_operator returns a PrimitiveScalar avoiding a DType allocation

  2. checked_sub delegates to checked_numeric_operator

As a consequence of (2), checked_sub now returns a None when there is an underflow/overflow
rather than returning a Null PrimitiveScalar (the return type is now
VortexResult<Option<PrimitiveScalar<'a>>>).

Moreover, std::ops::Sub now returns an error on underflow/overflow rather than a Null
PrimitiveScalar.

@danking
Copy link
Member

danking commented Dec 19, 2024

@doki23 does this suit your needs?

@doki23
Copy link
Contributor Author

doki23 commented Dec 19, 2024

@doki23 does this suit your needs?

Thanks, I'll have a look.

@doki23
Copy link
Contributor Author

doki23 commented Dec 19, 2024 •
edited
Loading

Hey @danking , thanks for your excellent work! I have only one suggestion — I prefer using a into method to convert PrimitiveScalar to Scalar rather than using to_scalar. If you also agree, I'll make this change.

@danking
Copy link
Member

danking commented Dec 19, 2024

Sounds good. I’ll approve with that change

@doki23
Copy link
Contributor Author

doki23 commented Dec 20, 2024

Sounds good. I’ll approve with that change

I've done, PTAL @danking

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

@gatesn gatesn gatesn left review comments

@danking danking Awaiting requested review from danking

At least 1 approving review is required to merge this pull request.

Assignees

@danking danking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@doki23 @gatesn @danking