-
Notifications
You must be signed in to change notification settings - Fork 912
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
Enable binops between Decimal and Integer columns #7859
base: branch-21.08
Are you sure you want to change the base?
Enable binops between Decimal and Integer columns #7859
Conversation
@@ -62,7 +62,46 @@ def to_arrow(self): | |||
buffers=[mask_buf, data_buf], | |||
) | |||
|
|||
def _from_integer_column( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why DecimalColumn._from_integer_column
instead of NumericalColumn.as_decimal_column
, which is already defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right about this. Updating.
@@ -218,6 +219,36 @@ def as_timedelta_column( | |||
), | |||
) | |||
|
|||
def _decimal_dtype(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems more suitable to go to utils/dtypes.py
, and naming should be decimal_dtype_from_numerical
?
lhs, rhs = rhs, lhs | ||
|
||
# result will be float, not decimal for these binops | ||
breakpoint() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stale breakpoint?
def decimal_series(input, dtype): | ||
return cudf.Series( | ||
[x if x is None else decimal.Decimal(x) for x in input], | ||
dtype=dtype, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be replaced by _decimal_series
Retargeted to |
Does this need to be moved to 21.10? |
@brandon-b-miller is this PR ready for review? |
No - there's a conceptual issue that needs to be resolved before we can move forward - see issue for details. |
Thanks Brandon! |
This PR has been labeled |
Closes #7680