-
Notifications
You must be signed in to change notification settings - Fork 323
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
Implement .floor, .ceil, .trunc for the in-memory Decimal column #10887
Conversation
apply_unary_operation self UnaryRoundOperation.CEIL_INSTANCE | ||
case self.inferred_precise_value_type.is_decimal of | ||
True -> | ||
apply_unary_operation self UnaryDecimalRoundOperation.CEIL_INSTANCE | ||
False -> | ||
apply_unary_operation self UnaryRoundOperation.CEIL_INSTANCE |
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.
Is there any reason why we cannot merge UnaryDecimalRoundOperation
with UnaryRoundOperation
?
I thought the whole idea behind the (relatively new) UnaryXyzOperation
architecture was to decouple operations from storages.
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.
But maybe I misunderstood the UnaryXyzOperation
approach. If you think that splitting them is better, then I think it is okay to keep this as-is.
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.
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.
Looks good to me, as written I'm wondering how we should approach our unary operations (as we probably will be moving binary ops to similar architecture some time in the future).
IMO there should be a single UnaryRoundOperation
. My motivation is - if we will migrate binary operations to this architecture, we'll prefer to have a single BinaryAddOperation
that is capable of handling all kinds of parameter combinations (int64 + float
, float + int32
etc. etc.) - if we had a separate class for each combination it would be unmanageable I think.
On the other hand, I do appreciate the 'decoupling' that keeping the separate unary implementations distinct. They are a slightly different case since they do not suffer from the combination of parameters problem.
What do you think?
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.