IgnatiusPang opened a new pull request, #25991: URL: https://github.com/apache/datafusion/pull/25991
> **Stacked on #25746.** This branch includes that PR's commits; only the last commit (`fix(functions): log does not preserve the order of inputs that may be negative`) belongs to this PR. I'll rebase onto `main` once #25746 merges. ## Which issue does this PR close? Follow-up to #25746, reported by @Toby1009 in https://github.com/apache/datafusion/pull/25746. ## Rationale for this change `LogFunc::output_ordering` passes through the ordering of `x` without checking that `x` is non-negative. `log_b(x)` is NaN for `x < 0`, and NaN sorts after every number, so `log` does not preserve the order of `x` across negative inputs, even with a base > 1. The optimizer then removes a sort that is still needed: ```sql SELECT x, log(2.0, x) AS l FROM (SELECT x FROM (VALUES (-1.0), (1.0), (4.0)) AS t(x) ORDER BY x LIMIT 3) ORDER BY l ASC; ``` Before: `l` = NaN, 0, 2. After: 0, 2, NaN. `ln`, `log2` and `log10` (`ln_order` etc. in `math/monotonicity.rs`) already only claim an ordering when their input is provably >= 0. `log` now makes the same check. `x = 0` is still fine, since `log_b(0) = -inf` sorts first. ## What changes are included in this PR? - `math/log.rs`: if `x` is ordered and its range is not provably >= 0, return `SortProperties::Unordered`. This applies to both `log(x)` and `log(b, x)`. Constant `x` keeps `Singleton`. - `order.slt`: added the reproducer above. The existing `log(2.0, x)` plan now keeps the outer sort, see below. ## Are these changes tested? Yes: new unit tests in `test_log_output_ordering` (a base > 1 with a possibly negative `x`, and `log(x)` with a possibly negative `x`), and the end-to-end query in `order.slt`. ## Are there any user-facing changes? Results only change for queries that were returning rows in the wrong order. However, a sorted column always gets an unbounded range in `get_expr_properties`, so `log(b, col)` on a plain sorted column no longer lets the optimizer remove a sort. This matches how `ln(col)` behaves today. The `log(2.0, x)` plan in `order.slt` that #25746 added as "the outer sort can be removed" now keeps the outer `SortExec`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
