Repository: apache/jena
Issue: NaN should not be same-value NaN

### Version

4.8.0

### What happened?

sameValue(NaN, NaN) is false.

notSameValue(NaN, NaN) is true.




### Relevant output and stacktrace

_No response_

### Are you interested in making a pull request?

Yes

Changed files:
jena-arq/src/test/java/org/apache/jena/sparql/expr/TestExpressionsMath.java
jena-arq/src/main/java/org/apache/jena/sparql/expr/ExprNode.java
jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValue.java
jena-arq/src/test/java/org/apache/jena/sparql/expr/TestLeviathanFunctions.java
jena-arq/src/test/java/org/apache/jena/sparql/function/library/TestFnFunctionsString.java
jena-arq/src/test/java/org/apache/jena/sparql/expr/TestNodeValue.java
jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValueCmp.java
jena-arq/src/test/java/org/apache/jena/sparql/expr/LibTestExpr.java

diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/expr/ExprNode.java b/jena-arq/src/main/java/org/apache/jena/sparql/expr/ExprNode.java
index bf70c052d5..26e89aca24 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/expr/ExprNode.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/expr/ExprNode.java
@@ -18,103 +18,107 @@
 
 package org.apache.jena.sparql.expr;
 
-import java.util.Set ;
+import java.util.Set;
 
-import org.apache.jena.sparql.algebra.Op ;
-import org.apache.jena.sparql.core.Var ;
-import org.apache.jena.sparql.engine.binding.Binding ;
-import org.apache.jena.sparql.expr.nodevalue.XSDFuncOp ;
-import org.apache.jena.sparql.function.FunctionEnv ;
+import org.apache.jena.sparql.algebra.Op;
+import org.apache.jena.sparql.core.Var;
+import org.apache.jena.sparql.engine.binding.Binding;
+import org.apache.jena.sparql.expr.nodevalue.XSDFuncOp;
+import org.apache.jena.sparql.function.FunctionEnv;
 import org.apache.jena.sparql.graph.NodeTransform;
-import org.apache.jena.sparql.sse.writers.WriterExpr ;
+import org.apache.jena.sparql.sse.writers.WriterExpr;
 
-/** 
+/**
  * A node that is a constraint expression that can be evaluated
  * An {@link Expr} is already a Constraint - ExprNode is the base implementation
  * of all {@link Expr} classes that provides the Constraint machinery.
  */
- 
+
 public abstract class ExprNode implements Expr
 {
     @Override
     public boolean isSatisfied(Binding binding, FunctionEnv funcEnv) {
         try {
-            NodeValue v = eval(binding, funcEnv) ;
-            boolean b = XSDFuncOp.booleanEffectiveValue(v) ;
-            return b ;
+            NodeValue v = eval(binding, funcEnv);
+            boolean b = XSDFuncOp.booleanEffectiveValue(v);
+            return b;
         }
-        catch (ExprEvalException ex) { 
-            return false ;
+        catch (ExprEvalException ex) {
+            return false;
         }
     }
 
-    public boolean isExpr()     { return true ; }
-    public final Expr getExpr() { return this ; }
-    
+    public boolean isExpr()     { return true; }
+    public final Expr getExpr() { return this; }
+
     // --- interface Constraint
-    
+
     @Override
-    public abstract NodeValue eval(Binding binding, FunctionEnv env) ; 
-    
+    public abstract NodeValue eval(Binding binding, FunctionEnv env);
+
     @Override
-    public final Set<Var> getVarsMentioned()                    { return ExprVars.getVarsMentioned(this) ; }
+    public final Set<Var> getVarsMentioned()                    { return ExprVars.getVarsMentioned(this); }
 
     @Override
-    public abstract int hashCode() ;
+    public abstract int hashCode();
+
     @Override
     public final boolean equals(Object other) {
-        if ( other == null ) return false ;
-        if ( this == other ) return true ;
-        if ( ! ( other instanceof Expr ) ) return false ;
-        return equals((Expr)other, false) ;
+        if ( other == null )
+            return false;
+        if ( this == other )
+            return true;
+        if ( !(other instanceof Expr) )
+            return false;
+        return equals((Expr)other, false);
     }
-    
+
     @Override
     public final boolean equalsBySyntax(Expr other) {
-        if ( other == null ) return false ;
-        if ( this == other ) return true ;
-        return equals(other, true) ;
+        if ( other == null ) return false;
+        if ( this == other ) return true;
+        return equals(other, true);
     }
-    
+
     @Override
-    public abstract boolean equals(Expr other, boolean bySyntax) ;
-    
-    protected static NodeValue eval(Binding binding, FunctionEnv funcEnv, Expr expr) {   
-        if ( expr == null ) return null ;
-        return expr.eval(binding, funcEnv) ;
+    public abstract boolean equals(Expr other, boolean bySyntax);
+
+    protected static NodeValue eval(Binding binding, FunctionEnv funcEnv, Expr expr) {
+        if ( expr == null ) return null;
+        return expr.eval(binding, funcEnv);
     }
-    
+
     @Override
-    final public Expr deepCopy()        { return copySubstitute(null) ; }
-    
+    final public Expr deepCopy()        { return copySubstitute(null); }
+
     @Override
-    public abstract Expr copySubstitute(Binding binding) ;
-    
+    public abstract Expr copySubstitute(Binding binding);
+
     @Override
-    public abstract Expr applyNodeTransform(NodeTransform transform) ;
-        
+    public abstract Expr applyNodeTransform(NodeTransform transform);
+
     // ---- Default implementations
     @Override
-    public boolean isVariable()         { return false ; }
+    public boolean isVariable()         { return false; }
     @Override
-    public String getVarName()          { return null ; } //throw new ExprException("Expr.getVarName called on non-variable") ; }
+    public String getVarName()          { return null; } //throw new ExprException("Expr.getVarName called on non-variable"); }
     @Override
-    public ExprVar getExprVar()         { return null ; } //throw new ExprException("Expr.getVar called on non-variable") ; }
+    public ExprVar getExprVar()         { return null; } //throw new ExprException("Expr.getVar called on non-variable"); }
     @Override
-    public Var asVar()                  { return null ; } //throw new ExprException("Expr.getVar called on non-variable") ; }
-    
+    public Var asVar()                  { return null; } //throw new ExprException("Expr.getVar called on non-variable"); }
+
     @Override
-    public boolean isConstant()         { return false ; }
+    public boolean isConstant()         { return false; }
     @Override
-    public NodeValue getConstant()      { return null ; } // throw new ExprException("Expr.getConstant called on non-constant") ; }
-    
+    public NodeValue getConstant()      { return null; } // throw new ExprException("Expr.getConstant called on non-constant"); }
+
     @Override
-    public boolean isFunction()         { return false ; }
+    public boolean isFunction()         { return false; }
     @Override
-    public ExprFunction getFunction()   { return null ; }
+    public ExprFunction getFunction()   { return null; }
 
-    public boolean isGraphPattern()     { return false ; }
-    public Op getGraphPattern()         { return null ; }
+    public boolean isGraphPattern()     { return false; }
+    public Op getGraphPattern()         { return null; }
     @Override
-    public String toString()            { return WriterExpr.asString(this) ; } 
+    public String toString()            { return WriterExpr.asString(this); }
 }
diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValue.java b/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValue.java
index d323019e6c..826f96f8da 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValue.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValue.java
@@ -487,7 +487,7 @@ public abstract class NodeValue extends ExprNode
      * the two NodeValues are known to be the same, else throw ExprEvalException
      */
     public static boolean notSameValueAs(NodeValue nv1, NodeValue nv2) {
-        return !sameValueAs(nv1, nv2);
+        return NodeValueCmp.notSameValueAs(nv1, nv2);
     }
 
     /** @deprecated Use {@link #sameValueAs(NodeValue, NodeValue)}. */
@@ -844,28 +844,25 @@ public abstract class NodeValue extends ExprNode
     public final String asQuotedString()
     { return asQuotedString(new SerializationContext()) ; }
 
-    public final String asQuotedString(SerializationContext context)
-    {
+    public final String asQuotedString(SerializationContext context) {
         // If possible, make a node and use that as the formatted output.
         if ( node == null )
-            node = asNode() ;
+            node = asNode();
         if ( node != null )
-            return FmtUtils.stringForNode(node, context) ;
-        return toString() ;
+            return FmtUtils.stringForNode(node, context);
+        return toString();
     }
 
-    // Convert to a string  - usually overridden.
-    public String asString()
-    {
+    // Convert to a string - usually overridden.
+    public String asString() {
         // Do not call .toString()
-        forceToNode() ;
-        return NodeFunctions.str(node) ;
+        forceToNode();
+        return NodeFunctions.str(node);
     }
 
     @Override
-    public int hashCode()
-    {
-        return asNode().hashCode() ;
+    public int hashCode() {
+        return asNode().hashCode();
     }
 
     @Override
@@ -873,7 +870,6 @@ public abstract class NodeValue extends ExprNode
         if ( other == null ) return false ;
         if ( this == other ) return true ;
         // This is the equality condition Jena uses - lang tags are different by case.
-
         if ( ! ( other instanceof NodeValue ) )
             return false ;
         NodeValue nv = (NodeValue)other ;
diff --git a/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValueCmp.java b/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValueCmp.java
index 9cf3993dc1..cfa40d2036 100644
--- a/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValueCmp.java
+++ b/jena-arq/src/main/java/org/apache/jena/sparql/expr/NodeValueCmp.java
@@ -55,12 +55,10 @@ public class NodeValueCmp {
 
         ValueSpace compType = NodeValue.classifyValueOp(nv1, nv2);
 
-        if ( nv1 == nv2 )
-            return true;
-        if ( nv1.hasNode() && nv2.hasNode() ) {
-            // Fast path - same RDF term => sameValue
-            if ( nv1.getNode().equals(nv2.getNode()) )
-                return true;
+        if ( nv1.equals(nv2) ) {
+            // Fast path - same RDF term => sameValue (except NaN).
+            if ( nv1.getNode() != null && nv2.getNode() != null && nv1.getNode().equals(nv2.asNode()) )
+                return sameExceptNaN(nv1, nv2);
         }
 
         // Special case - date/dateTime comparison is affected by timezones and may
@@ -152,6 +150,15 @@ public class NodeValueCmp {
         throw new ARQInternalErrorException("sameValueAs failure " + nv1 + " and " + nv2);
     }
 
+    // When nv1 and nv1 are known to be the sameTerm, including java ==
+    private static boolean sameExceptNaN(NodeValue nv1, NodeValue nv2) {
+        if ( nv1.isDouble() && Double.isNaN(nv1.getDouble()) )
+            return false;
+        if ( nv1.isFloat() && Float.isNaN(nv1.getFloat()) )
+            return false;
+        return true;
+    }
+
     /** Worker for sameAs. */
     private static boolean nSameValueAs(Node n1, Node n2) {
         NodeValue nv1 = NodeValue.makeNode(n1);
@@ -171,8 +178,9 @@ public class NodeValueCmp {
      * Return true if the two NodeValues are known to be different, return false if
      * the two NodeValues are known to be the same, else throw ExprEvalException
      */
-    /*package*/private static boolean notSameValueAs(NodeValue nv1, NodeValue nv2) {
-        return !sameValueAs(nv1, nv2);
+    /*package*/ static boolean notSameValueAs(NodeValue nv1, NodeValue nv2) {
+        // Works for NaN as well.
+        return  !sameValueAs(nv1, nv2);
     }
 
     // ==== Compare
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/expr/LibTestExpr.java b/jena-arq/src/test/java/org/apache/jena/sparql/expr/LibTestExpr.java
index 7b01f8a38b..4a57eb62b0 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/expr/LibTestExpr.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/expr/LibTestExpr.java
@@ -92,6 +92,16 @@ public class LibTestExpr {
         assertTrue("Expected = " + expected + " : Actual = " + actual, sameValueSameDatatype(expected, actual));
     }
 
+    public static void testIsNaN(String exprStr) {
+        testSameObject(exprStr, NodeValue.nvNaN);
+    }
+
+    public static void testSameObject(String exprStr, NodeValue expected) {
+        Expr expr = parse(exprStr);
+        NodeValue actual = expr.eval(null, LibTestExpr.createTest());
+        assertTrue("Expected = " + expected + " : Actual = " + actual, expected.equals(actual));
+    }
+
     private static boolean sameValueSameDatatype(NodeValue nv1, NodeValue nv2) {
         if ( ! NodeValue.sameValueAs(nv1, nv2) )
             return false;
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestExpressionsMath.java b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestExpressionsMath.java
index c323bb4993..32cb43375c 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestExpressionsMath.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestExpressionsMath.java
@@ -36,7 +36,7 @@ public class TestExpressionsMath
     @Test public void exp_04()          { test("math:exp(1e0/0)", "'INF'^^xsd:double") ; }
     @Test public void exp_05()          { test("math:exp('INF'^^xsd:double)", "'INF'^^xsd:double") ; }
     @Test public void exp_06()          { test("math:exp('-INF'^^xsd:double)", "'0.0e0'^^xsd:double") ; }
-    @Test public void exp_07()          { test("math:exp('NaN'^^xsd:double)", "'NaN'^^xsd:double") ; }
+    @Test public void exp_07()          { testIsNaN("math:exp('NaN'^^xsd:double)") ; }
 
     @Test public void exp10_01()        { test("math:exp10(2)", "100") ; }
     @Test public void exp10_02()        { testDouble("math:exp10(-1)", 0.1, 0.00001) ; }
@@ -50,7 +50,7 @@ public class TestExpressionsMath
     @Test public void log_04()          { test("math:log(0)", "'-INF'^^xsd:double") ; }
     @Test public void log_05()          { test("math:exp('INF'^^xsd:double)", "'INF'^^xsd:double") ; }
     @Test public void log_06()          { test("math:exp('-INF'^^xsd:double)", "'0.0e0'^^xsd:double") ; }
-    @Test public void log_07()          { test("math:exp('NaN'^^xsd:double)", "'NaN'^^xsd:double") ; }
+    @Test public void log_07()          { testIsNaN("math:exp('NaN'^^xsd:double)") ; }
 
     @Test public void pow_01()          { test("math:pow(2,2)", "4") ; }
     @Test public void pow_02()          { testDouble("math:pow(2,-2)", 0.25, 0.00001) ; }
@@ -62,19 +62,19 @@ public class TestExpressionsMath
 
     @Test public void pow_13()          { test("math:pow('INF'^^xsd:double,0)", "'1.0e0'^^xsd:double") ; }
     @Test public void pow_14()          { test("math:pow('-INF'^^xsd:double, 0)", "'1.0e0'^^xsd:double") ; }
-    @Test public void pow_15()          { test("math:pow('NaN'^^xsd:double, 1)", "'NaN'^^xsd:double") ; }
-    @Test public void pow_16()          { test("math:pow(1, 'NaN'^^xsd:double)", "'NaN'^^xsd:double") ; }
+    @Test public void pow_15()          { testIsNaN("math:pow('NaN'^^xsd:double, 1)") ; }
+    @Test public void pow_16()          { testIsNaN("math:pow(1, 'NaN'^^xsd:double)") ; }
 
     @Test public void sqrt_01()         { test("math:sqrt(1)", "'1.0e0'^^xsd:double") ; }
     @Test public void sqrt_02()         { testDouble("math:sqrt(2)", Math.sqrt(2), 0.000001) ; }
-    @Test public void sqrt_03()         { test("math:sqrt(-2)", "'NaN'^^xsd:double") ; }
+    @Test public void sqrt_03()         { testIsNaN("math:sqrt(-2)"); }
 
     @Test(expected=ARQException.class)
     public void sqrt_04()               { test("math:sqrt('TWO')", "'dummy'") ; }
 
     @Test public void sqrt_10()         { test("math:sqrt('INF'^^xsd:double)", "'INF'^^xsd:double") ; }
-    @Test public void sqrt_11()         { test("math:sqrt('-INF'^^xsd:double)", "'NaN'^^xsd:double") ; }
-    @Test public void sqrt_12()         { test("math:sqrt('NaN'^^xsd:double)", "'NaN'^^xsd:double") ; }
+    @Test public void sqrt_11()         { testIsNaN("math:sqrt('-INF'^^xsd:double)") ; }
+    @Test public void sqrt_12()         { testIsNaN("math:sqrt('NaN'^^xsd:double)") ; }
 
     //  4.8.7 math:sqrt
     //  4.8.8 math:sin
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestLeviathanFunctions.java b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestLeviathanFunctions.java
index 6bbdda0556..0e3b6ed447 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestLeviathanFunctions.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestLeviathanFunctions.java
@@ -21,6 +21,7 @@ package org.apache.jena.sparql.expr;
 import static org.apache.jena.sparql.expr.LibTestExpr.test ;
 import static org.apache.jena.sparql.expr.LibTestExpr.testDouble ;
 import static org.apache.jena.sparql.expr.LibTestExpr.testError ;
+import static org.junit.Assert.assertTrue;
 
 import org.apache.jena.sparql.util.NodeFactoryExtra ;
 import org.junit.AfterClass;
@@ -130,7 +131,9 @@ public class TestLeviathanFunctions {
 
     @Test
     public void log_03() {
-        test("lfn:log(-1)", NodeFactoryExtra.doubleToNode(Double.NaN));
+        NodeValue actual = LibTestExpr.eval("lfn:log(-1)");
+        // Test the object, not the value.
+        assertTrue(NodeValue.nvNaN.equals(actual));
     }
 
     @Test
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestNodeValue.java b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestNodeValue.java
index 8da966aa31..ff2134a1d4 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestNodeValue.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/expr/TestNodeValue.java
@@ -1067,6 +1067,51 @@ public class TestNodeValue
         assertFalse("Different values - notNotSame (" + nv1 + "," + nv3 + ")", NodeValue.notSameValueAs(nv1, nv3));
     }
 
+    @Test
+    public void testSameValueNaN_double_1() {
+        NodeValue nv1 = NodeValue.makeNode("NaN", XSDDatatype.XSDdouble);
+        NodeValue nv2 = NodeValue.makeNode("NaN", XSDDatatype.XSDdouble);
+        assertEquals(nv1, nv2);
+        assertFalse(NodeValue.sameValueAs(nv1, nv2));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv2));
+    }
+
+    @Test
+    public void testSameValueNaN_float_1() {
+        NodeValue nv1 = NodeValue.makeNode("NaN", XSDDatatype.XSDfloat);
+        NodeValue nv2 = NodeValue.makeNode("NaN", XSDDatatype.XSDfloat);
+        assertEquals(nv1, nv2);
+        assertFalse(NodeValue.sameValueAs(nv1, nv2));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv2));
+        // NaN is weird.
+        assertFalse(NodeValue.sameValueAs(nv1, nv1));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv1));
+    }
+
+    @Test
+    public void testSameValueNaN_double_2() {
+        NodeValue nv1 = NodeValue.makeDouble(Double.NaN);
+        NodeValue nv2 = NodeValue.makeDouble(Double.NaN);
+        assertEquals(nv1, nv2);
+        assertFalse(NodeValue.sameValueAs(nv1, nv2));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv2));
+
+        assertFalse(NodeValue.sameValueAs(nv1, nv1));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv1));
+    }
+
+    @Test
+    public void testSameValueNaN_float_2() {
+        NodeValue nv1 = NodeValue.makeFloat(Float.NaN);
+        NodeValue nv2 = NodeValue.makeFloat(Float.NaN);
+        assertEquals(nv1, nv2);
+        assertFalse(NodeValue.sameValueAs(nv1, nv2));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv2));
+
+        assertFalse(NodeValue.sameValueAs(nv1, nv1));
+        assertTrue(NodeValue.notSameValueAs(nv1, nv1));
+    }
+
     @Test
     public void testLang1() {
         Node n1 = org.apache.jena.graph.NodeFactory.createLiteral("xyz", "en");
diff --git a/jena-arq/src/test/java/org/apache/jena/sparql/function/library/TestFnFunctionsString.java b/jena-arq/src/test/java/org/apache/jena/sparql/function/library/TestFnFunctionsString.java
index 67dce3e37f..af91d6ab0f 100644
--- a/jena-arq/src/test/java/org/apache/jena/sparql/function/library/TestFnFunctionsString.java
+++ b/jena-arq/src/test/java/org/apache/jena/sparql/function/library/TestFnFunctionsString.java
@@ -92,7 +92,7 @@ public class TestFnFunctionsString {
 
     @Test public void exprStrNormalizeSpace0() { test("fn:normalize-space(' The    wealthy curled darlings                                         of    our    nation. ')",
             NodeValue.makeString("The wealthy curled darlings of our nation.")) ; }
-    @Test public void exprStrNormalizeSpace1() { test("fn:normalize-space('')",NodeValue.nvEmptyString) ; }
+    @Test public void exprStrNormalizeSpace1() { test("fn:normalize-space('')", NodeValue.nvEmptyString) ; }
     @Test public void exprStrNormalizeSpace2() { test("fn:normalize-space('   Aaa     ')",NodeValue.makeString("Aaa")) ; }
     @Test public void exprStrNormalizeSpace3() { test("fn:normalize-space('A a   a    a a    ')",NodeValue.makeString("A a a a a")) ; }