carloea2 commented on code in PR #8709:
URL: https://github.com/apache/texera/pull/8709#discussion_r4225229004
##########
common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/sort/StableMergeSortOpDesc.scala:
##########
@@ -69,4 +73,51 @@ class StableMergeSortOpDesc extends LogicalOp {
List(InputPort()),
List(OutputPort(blocking = true))
)
+
+ // The engine runs an incremental stable merge sort with nulls last whichever
+ // way a key points. pandas' mergesort is stable too, so the ordering below
is
+ // the same one.
+ //
+ // A string column parts more narrowly: the engine reads UTF-16 code units
and
+ // pandas reads code points, which agree below U+FFFF and can differ above
it.
+ override def generateStandaloneCode(): String = {
+ val criteria = Option(keys).getOrElse(ListBuffer.empty)
+ if (criteria.isEmpty) return "out1df = in1df.copy()"
+ val cols = criteria
+ .map(c => pyStringLiteral(c.attributeName))
+ .mkString("[", ", ", "]")
+ val ascending = criteria
+ .map(c => if (c.sortPreference == SortPreference.ASC) "True" else
"False")
+ .mkString("[", ", ", "]")
+ // Sort each key in three tiers, because the engine treats a null and a NaN
+ // differently: a null goes last whichever way the key points, while a NaN
+ // compares above every number, so it goes last ascending and first
+ // descending. A column read into a numpy dtype has one slot for both, and
+ // there both land in the null tier, which is where they were before.
+ //
+ // The tiers are built in a frame of their own and named by position, so
+ // the input keeps every column it arrived with: a helper named after the
+ // key would overwrite an input column that already answers to that name,
+ // and dropping the helper afterwards would take the payload with it.
+ s"""_texera_sorted = in1df.reset_index(drop=True)
+ |_texera_keys = pd.DataFrame(index=_texera_sorted.index)
+ |_texera_by = []
+ |_texera_asc = []
+ |for _texera_i, (_texera_col, _texera_a) in enumerate(zip($cols,
$ascending)):
+ | _texera_null = "null_" + str(_texera_i)
+ | _texera_nan = "nan_" + str(_texera_i)
+ | _texera_val = "val_" + str(_texera_i)
+ | _texera_keys[_texera_null] = _texera_sorted[_texera_col].isna()
+ | _texera_keys[_texera_nan] = (
+ | _texera_sorted[_texera_col] != _texera_sorted[_texera_col]
+ | ).fillna(False)
+ | _texera_keys[_texera_val] = _texera_sorted[_texera_col]
Review Comment:
Please preserve the native string ordering here. Sorting U+FF01 and U+1F600
ascending puts U+1F600 first in StableMergeSortOpExec, but the generated script
puts U+FF01 first. I ran both on the same input; an ASCII control passes. These
are valid string values, so a downstream Limit can select a different row after
export. Use a UTF-16 sort key and add a parity test.
--
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]
