-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Added MIN/MAX aggregate function compatible with char/varchar #1739
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
Conversation
| struct AggregateFuncTraits<OLAP_FIELD_AGGREGATION_MIN, OLAP_FIELD_TYPE_VARCHAR> : | ||
| public AggregateFuncTraits<OLAP_FIELD_AGGREGATION_NONE, OLAP_FIELD_TYPE_VARCHAR> { | ||
| static void update(RowCursorCell* dst, const RowCursorCell& src, Arena* arena) { | ||
| bool src_null = src.is_null(); |
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.
I think is dst is null, you can just return, because null is the mimimum value.
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.
We want to ignore null for comparison. We want null to be always replaced.
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.
Oh, I see~
|
|
||
| Slice* dst_slice = reinterpret_cast<Slice*>(dst->mutable_cell_ptr()); | ||
| const Slice* src_slice = reinterpret_cast<const Slice*>(src.cell_ptr()); | ||
| if (dst_null || src_slice->compare(*dst_slice) < 0) { |
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.
I this this if condition should be:
if (!dst_null && src_slice->compare(*dst_slice) < 0)
Did I misunderstand?
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.
Same here. Null always be replaced.
morningman
left a comment
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.
LGTM
| struct AggregateFuncTraits<OLAP_FIELD_AGGREGATION_MIN, OLAP_FIELD_TYPE_VARCHAR> : | ||
| public AggregateFuncTraits<OLAP_FIELD_AGGREGATION_NONE, OLAP_FIELD_TYPE_VARCHAR> { | ||
| static void update(RowCursorCell* dst, const RowCursorCell& src, Arena* arena) { | ||
| bool src_null = src.is_null(); |
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.
Oh, I see~
…n of DECIMAL (apache#1739) FE saves DECIMALV2 as DECIMALV2, however BE saves DECIMALV2 as DECIMAL http://jira.selectdb-in.cc/browse/CORE-2017
No description provided.