Repository navigation
Fix : signum function bug when 0.0 input - #11580
Merged
Merged
Conversation
andygrove
approved these changes
Jul 22, 2024
andygrove
left a comment
andygrove
left a comment
Member
There was a problem hiding this comment.
LGTM for a Postgres-compatible implementation of signum.
For spark compatibility we would need a small change to handle the -0.0 case to return -1. We could either implement a copy of this code in the Comet project, or add a flag to the constructor of this function to control behavior for the -0.0 special case.
|
Here are the values I get for Spark 3.5.1: data = ["-1", "1", "0", "-0.0", "0.0", "1.0", "-1.0"]
df = spark.createDataFrame([(datum,) for datum in data], "col1 string")
df.collect()
# [Row(col1='-1'), Row(col1='1'), Row(col1='0'), Row(col1='-0.0'), Row(col1='0.0'), Row(col1='1.0'), Row(col1='-1.0')]
pprint.pprint(df.selectExpr("col1", "signum(cast(col1 as float)) as result").collect())
# [Row(col1='-1', result=-1.0),
# Row(col1='1', result=1.0),
# Row(col1='0', result=0.0),
# Row(col1='-0.0', result=-0.0),
# Row(col1='0.0', result=0.0),
# Row(col1='1.0', result=1.0),
# Row(col1='-1.0', result=-1.0)]So it seems like -0.0 in Spark should specifically return -0.0 in the signum function, not -1, right? Literals seem to be treated as Decimal instead, which doesn't make the distinction between negative and positive zero, so those don't see the same behavior - I'd guess that's what caused the specific results listed for Spark in #11557 (comment): spark.sql("""SELECT -1, 1, 0, -0.0, 0.0, 1.0, -1.0""").collect()
# [Row(-1=-1, 1=1, 0=0, 0.0=Decimal('0.0'), 0.0=Decimal('0.0'), 1.0=Decimal('1.0'), -1.0=Decimal('-1.0'))]
spark.sql("""SELECT signum(-1), signum(1), signum(0), signum(-0.0), signum(0.0), signum(1.0), signum(-1.0)""").collect()
# [Row(SIGNUM(-1)=-1.0, SIGNUM(1)=1.0, SIGNUM(0)=0.0, SIGNUM(0.0)=0.0, SIGNUM(0.0)=0.0, SIGNUM(1.0)=1.0, SIGNUM(-1.0)=-1.0)]For Postgres 16.3, this behavior looks like what I see for both postgres=# WITH tab(datum) AS (VALUES ('-1'), ('1'), ('0'), ('-0.0'), ('0.0'), ('1.0'), ('-1.0'))
SELECT *, cast(datum as decimal) decimal, cast(datum as float) float, sign(cast(datum as decimal)) sign_decimal, sign(cast(datum as float)) sign_float FROM tab;
datum | decimal | float | sign_decimal | sign_float
-------+---------+-------+--------------+------------
-1 | -1 | -1 | -1 | -1
1 | 1 | 1 | 1 | 1
0 | 0 | 0 | 0 | 0
-0.0 | 0.0 | -0 | 0 | 0
0.0 | 0.0 | 0 | 0 | 0
1.0 | 1.0 | 1 | 1 | 1
-1.0 | -1.0 | -1 | -1 | -1
(7 rows) |
Contributor
|
Thanks @getChan @andygrove and @Throne3d |
lyne7-sc
pushed a commit
to lyne7-sc/datafusion
that referenced
this pull request
Aug 14, 2026
## Which issue does this PR close? N/A — documentation-only correction; no open issue tracks it. ## Rationale for this change The user guide for `signum` says "Zero and positive numbers return `1`", but `signum(0)` returns `0` — the documented return value for zero is the opposite of what the function does. The repository's own tests already pin the real behavior: - `datafusion/sqllogictest/test_files/scalar.slt`: `select signum(-2), signum(0), signum(2);` → `-1 0 1` - The unit tests in `signum.rs` assert `signum(±0.0) == 0.0` That behavior is intentional: it matches PostgreSQL `sign()` and Spark `signum()`, and was changed on purpose in apache#11580 (issue apache#11557). Only the docs were left describing the pre-apache#11580 behavior, so this PR updates the docs, not the code. ## What changes are included in this PR? Documentation only: - Update the `#[user_doc]` description in `datafusion/functions/src/math/signum.rs` to state that zero returns `0`. - Regenerate `docs/source/user-guide/sql/scalar_functions.md` via `dev/update_function_docs.sh`. ## Are these changes tested? No new tests: the corrected wording describes behavior the existing tests above already assert, and those are what it was checked against. - `cargo test -p datafusion-functions --lib math::signum` → 2 passed - `cargo fmt --all -- --check` and `./ci/scripts/doc_prettier_check.sh` → clean ## Are there any user-facing changes? Yes, documentation only. No API or runtime behavior change. --- Prepared with AI assistance; every claim above was verified locally against the repository's existing tests before opening this PR. Co-authored-by: codeAnqiang-ma <[email protected]>
imtherealnaska
pushed a commit
to imtherealnaska/datafusion
that referenced
this pull request
Aug 16, 2026
## Which issue does this PR close? N/A — documentation-only correction; no open issue tracks it. ## Rationale for this change The user guide for `signum` says "Zero and positive numbers return `1`", but `signum(0)` returns `0` — the documented return value for zero is the opposite of what the function does. The repository's own tests already pin the real behavior: - `datafusion/sqllogictest/test_files/scalar.slt`: `select signum(-2), signum(0), signum(2);` → `-1 0 1` - The unit tests in `signum.rs` assert `signum(±0.0) == 0.0` That behavior is intentional: it matches PostgreSQL `sign()` and Spark `signum()`, and was changed on purpose in apache#11580 (issue apache#11557). Only the docs were left describing the pre-apache#11580 behavior, so this PR updates the docs, not the code. ## What changes are included in this PR? Documentation only: - Update the `#[user_doc]` description in `datafusion/functions/src/math/signum.rs` to state that zero returns `0`. - Regenerate `docs/source/user-guide/sql/scalar_functions.md` via `dev/update_function_docs.sh`. ## Are these changes tested? No new tests: the corrected wording describes behavior the existing tests above already assert, and those are what it was checked against. - `cargo test -p datafusion-functions --lib math::signum` → 2 passed - `cargo fmt --all -- --check` and `./ci/scripts/doc_prettier_check.sh` → clean ## Are there any user-facing changes? Yes, documentation only. No API or runtime behavior change. --- Prepared with AI assistance; every claim above was verified locally against the repository's existing tests before opening this PR. Co-authored-by: codeAnqiang-ma <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Closes #11557
What changes are included in this PR?
signum_orderfunction from monotonicity.rs to signum.rsmake_udf_function!macroAre these changes tested?
Are there any user-facing changes?
no