andygrove commented on PR #5469: URL: https://github.com/apache/datafusion-comet/pull/5469#issuecomment-5441291460
> **Note on this review:** this was generated by an LLM (Claude Code) at my request while I worked through a review backlog. I have not verified the individual findings myself. Please treat everything below as suggestions to evaluate rather than as authoritative review feedback, and push back on anything that is wrong or already handled. This is a thorough piece of work. Reusing `NormalizeNaNAndZero` rather than inventing a new comparator is the right call, and I like that `create_normalized_key_expr` is shared by sort, window partition keys, window group limit, and range partitioning so the four cannot drift apart. The `floating_window_partition_keys_preserve_ordering` test that checks `get_ordered_partition_by_indices` still returns the prefix is a good catch, since silently losing the sorted window mode would have been an easy regression to miss. Flipping the `ignore(...)` in `window_group_limit_rank.sql` back to a real assertion is nice to see. A few things I would like resolved before merge. **No performance data for the sort path** `create_sort_expr` now wraps every scalar `FLOAT` and `DOUBLE` sort key, which means `native/core/src/execution/planner.rs:1498` adds a full column copy per batch to every native sort on a floating key, plus the same for window ordering and range partitioning. `unary` allocates a fresh values buffer each time. That is probably fine relative to the row encoding and the sort itself, but "probably fine" is not a number. Could you run a sort benchmark with a float key before and after and put the result in the description? If it turns out to be measurable, it would be worth discussing whether the normalization can be folded into the row encoder instead of materializing an intermediate array. **`strictFloatingPoint` now falls back for cases this PR fixes** The docs change says the `spark.comet.exec.strictFloatingPoint=true` fallback policy is unchanged, "including its conservative fallback for scalar floating-point sort keys." After this PR those scalar keys agree with Spark, so users who turn strict mode on are paying a fallback for something that is now correct. Is narrowing that fallback to the nested array and struct cases in scope here, or should it be a follow-up? If it is a follow-up I would like an issue filed and linked from this PR, because a stale conservative fallback is the kind of thing that quietly stays for a year. **Tracking issue for the nested case** Same question for the nested array and struct divergence the docs now call out. There is no issue link in the new `floating-point.md` text. A user who reads that paragraph and is affected has nowhere to go. Could you file one and link it from the doc? **Sort-merge join builds an expression it throws away** At `planner.rs:2145` the `create_sort_expr` result is used only for `sort_expr.options`, and the expression itself is discarded. That call now constructs a `NormalizeNaNAndZero` node for nothing on float keys. It is not a correctness problem, but it does suggest that site wants a smaller helper that only decodes direction and null ordering. Worth splitting out? **Silent passthrough in `normalize_array`** `NormalizeNaNAndZero::normalize_array` returns the input untouched for any type that is not `Float32` or `Float64`, including `Dictionary(_, Float64)`, `List<Float64>`, and structs with float fields. In the range-partitioning call site that is consistent with `create_normalized_key_expr`, which also only handles the two scalar types, so I do not think there is a bug here today. But the function is `pub` and the passthrough is silent. Could the doc comment say explicitly that nested and dictionary-encoded floats are deliberately not normalized, and that callers must match `create_normalized_key_expr`'s coverage? Otherwise the next caller gets a silently wrong answer with no compile error. **One test question** In `CometWindowExecSuite`, the new tests set `COMET_EXEC_STRICT_FLOATING_POINT` to `false` explicitly, which is already the default. Is that there to document intent, or to guard against the default flipping? If the former, a short comment saying so would help, since otherwise it reads like the tests only pass in non-strict mode. -- 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]
