amoeba commented on code in PR #1430:
URL: https://github.com/apache/arrow-go/pull/1430#discussion_r4404326768
##########
arrow/compute/internal/kernels/vector_sort.go:
##########
@@ -53,11 +53,19 @@ func (SortOptions) TypeName() string { return "SortOptions"
}
type SortState = SortOptions
-// SortKey defines a column to sort by with its ordering and null placement
options.
+// SortKey defines a column to sort by with its ordering, null placement and
float ordering options.
type SortKey struct {
ColumnIndex int
Order SortOrder
NullPlacement NullPlacement
+ // FloatTotalOrder orders float16/float32/float64 columns by the IEEE
754 totalOrder
+ // predicate: -NaN < -Inf < negative values < -0 < +0 < positive values
< +Inf < +NaN.
+ // NaNs are ordered as values (not null-like) and Descending reverses
the whole order,
+ // NaNs included; nulls still follow NullPlacement. When false (the
default), floats
+ // compare numerically with -0 == +0 and every NaN, regardless of sign,
is grouped next
+ // to the nulls according to NullPlacement, independent of Order (Arrow
C++ behavior).
+ // Non-float columns ignore it.
+ FloatTotalOrder bool
Review Comment:
Hrm, should we add benchmarks...
##########
arrow/compute/internal/kernels/vector_sort_physical.go:
##########
@@ -444,6 +450,9 @@ func (c *physicalSortFloat16Column) compareRowsForKey(i, j
uint64, key SortKey)
return v
}
}
+ if c.totalOrder {
+ return compareOrdered(key.Order,
totalOrderKey16(a.Value(li).Uint16()), totalOrderKey16(b.Value(lj).Uint16()))
+ }
Review Comment:
This PR adds branches in three places to check `totalOrder`. I had Opus 5.5
run a benchmark and it looks like this PR slows down the old path by 2-3% and
maybe it's due to these branches?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
