cloud-fan commented on code in PR #58234:
URL: https://github.com/apache/spark/pull/58234#discussion_r4068644402
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/PivotFirst.scala:
##########
@@ -87,13 +87,25 @@ case class PivotFirst(
override val dataType: DataType = ArrayType(valueDataType)
- private val usesTreeMap: Boolean =
!TypeUtils.typeWithProperEquals(pivotColumn.dataType)
+ private val usesOrderingBasedIndex: Boolean = pivotColumn.dataType match {
+ case FloatType | DoubleType => true
Review Comment:
**Non-blocking (P2):** Routing every Float/Double pivot through TreeMap
makes `PivotFirst.update` pay O(log K) for every input row, where the previous
HashMap path was expected O(1). We can preserve the corrected SQL equality
without that regression by hashing a canonical key: normalize NaN and signed
zero and use `doubleToLongBits`/`floatToIntBits` as the existing `CollectSet`
path does, while leaving `null` as a valid HashMap key. Please keep
ordering-based lookup for types that actually need it and canonicalize floating
keys on both index construction and row lookup.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
