zeroshade commented on code in PR #1430:
URL: https://github.com/apache/arrow-go/pull/1430#discussion_r4405254084
##########
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:
Good catch on the branches — addressed in 27dfedee by not having them in the
default path at all: `FloatTotalOrder` now selects separate comparator types
(`physicalSortFloat{16,32,64}TotalOrderColumn`) once in
`createChunkedComparator`, so the default float comparators are byte-for-byte
the same as on `main`. The only remaining check is the one in the single-key
radix condition, evaluated once per `SortIndices` call.
For what it's worth, I couldn't reproduce a 2–3% difference on the previous
head either: interleaved runs of `main` and the PR (pinned core,
`GOMAXPROCS=1`, 8 rounds) showed every default-path case within noise (geomean
+0.26%), but the noise floor was ~5%, so the restructure removes the question
rather than relying on that.
##########
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:
Added `BenchmarkSortIndices_Float64` in 27dfedee (65,536 rows; one chunk,
one chunk with NaNs, 16 chunks, and 16 chunks with nulls + NaNs; each with
`totalOrder=false/true`). On amd64 (pinned, `GOMAXPROCS=1`, 8 rounds), total
order vs the default:
| case | default | total order |
|---|---|---|
| 1 chunk (radix path) | 20.3 ms | 2.4 ms (−88%), +0.5 MiB scratch |
| 1 chunk, NaNs (radix path) | 20.3 ms | 2.2 ms (−89%) |
| 16 chunks (comparator path) | 19.9 ms | 14.1 ms (−29%) |
| 16 chunks, nulls + NaNs | 24.5 ms | 18.9 ms (−23%) |
The default path is unchanged vs `main` (same comparator code).
##########
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:
Yes — documented on `SortKey.FloatTotalOrder` in 27dfedee: total order is
usually faster than the default (integer keys, no NaN checks), and with a
single key on a single chunk of ≥2048 rows without nulls it becomes a radix
sort that allocates a temporary key buffer the size of the column's values.
Numbers are in the benchmarks thread.
##########
arrow/compute/internal/kernels/vector_sort.go:
##########
@@ -161,8 +169,8 @@ func newFixedSizeBinaryComparator(chunks []arrow.Array,
numRows int, vn bool, ro
}
// createChunkedComparator builds a column comparator for these chunks (one
Arrow type for all chunks).
-// rowMap may be shared by aligned multi-key columns.
-func createChunkedComparator(chunks []arrow.Array, numRows int, rowMap
logicalRowMap) (columnComparator, error) {
+// rowMap may be shared by aligned multi-key columns. floatTotalOrder selects
IEEE 754 totalOrder for floats.
Review Comment:
Took the suggestion in 27dfedee — "selects" isn't used for options anywhere
else.
--
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]
