refactor: move column operations to ColumnOperation trait
#222
Conversation
|
@JayWhite2357 can I get a reviewer assigned |
| right_type: rhs, | ||
| }); | ||
| /// A trait for column operations. | ||
| pub trait ColumnOperation { |
There was a problem hiding this comment.
@ssddOnTop Hmm this looks like a misnomer. What's your goal here, may I ask?
There was a problem hiding this comment.
@iajoiner I might be confused here but I was thinking that now we will have the normal Column and the NullableColumn so we could implement a trait for each type so that we could perform dynamic dispatch to avoid massive changes in future.
There was a problem hiding this comment.
I think @iajoiner's point here is that you have impl ColumnOperation for ColumnType. So, it probably should be named ColumnTypeOperation.
My instinct is that a trait doesn't make sense because it's not clear what the other type that would implement it would be. ColumnType and NullableColumnType seems to be what you are thinking.
It's not clear that NullableColumnType is the right design choice here.
Even if this refactor makes sense, I would like to see other PRs come first that necessitate this refactor.
|
@ssddOnTop are you waiting on us here? I don't see activity on this. |
I was waiting for a reply on #222 (comment) I am finding it hard to communicate here.. |
Rationale for this change
Now that we are including nullable column support in #183, the comment #183 (comment) suggests creating new structs for nullable columns
This trait will help us with the easy implementation of methods
What changes are included in this PR?
I introduced a trait
ColumnOperationwhich contains the functionstry_add_subtract_column_types,try_multiply_column_typesandtry_divide_column_typesAre these changes tested?
The refactor does not affect any core logic, and changes made are covered under the tests.
PS: ext of #194