DecimalNode and equality: on purpose or not, I don't know, but...

[fge]

If I create an ObjectMapper with feature DeserializationFeature.USE_BIG_DECIMAL_FOR_FLOATS, and create two nodes as follows:

// mapper is the ObjectMapper
final JsonNode n1 = mapper.readTree("1.0");
final JsonNode n2 = mapper.readTree("1.00");

then ·n1.equals(n2) returns false. BigDecimal is quite awkward in this regard since its .compareTo() is not consistent with .equals() (and the two BigDecimals created with the two strings above are actually not equal). But now, is this the correct thing to do?

I extracted DecimalNode from the source code and tried, in the .equals() method, to replace .equals() with .compareTo() == 0 for the decimal values -- but that did not work!

What did work and rendered these two nodes equal was to modify constructors so that they read:

    public DecimalNode(BigDecimal v)
    {
        _value = v.stripTrailingZeros();
    }

    public static DecimalNode valueOf(BigDecimal d)
    {
        return new DecimalNode(d);
    }

I am not sure whether this is the desired behaviour for DecimalNode, however. But what I don't understand is why replacing .equals(_value) with .compareTo(_value) == 0 in DecimalNode's .equals() didn't work...

[cowtowncoder]

It sounds unfortunate. Thank you for reporting this.
I agree in that behavior currently is not intuitive and should be improved.

Whether we would want to normalize values or not is probably something that needs to be another Feature. Mostly because I think that there are others who would prefer keeping original representation if possible (i.e. not remove trailing zeroes).

As to new feature: adding it might be easy, as long as JsonNodeFactory has enough information passed; whether it does I'll have to check. If not, it would have to be something to configure on factory object, meaning bit more work (construct with configuration, pass to ObjectMapper).

And then there is the question of default behavior. The usual answer is to do what previous version does, and maybe that is the way to go. But at the same time, I think it's more likely that new users would find intuitive equality comparison to be more useful than keeping extra trailing zeroes around (assuming they are even retained?).

[fge]

I think it's more likely that new users would find intuitive equality comparison to be more useful than keeping extra trailing zeroes around (assuming they are even retained?).

Well, yes they are. 1.0, for instance, has scale 1, while 1.00 has scale 2. And I guess this is why they are not considered equal wrt BigDecimal (I haven't had a look at the code yet).

In fact, this is true for Sun's JDK, I have not tested with, for instance, OpenJDK.

EDIT: confirmed, and the Javadoc also says so. And this is also why 1e0 equals to 1 since both have scale 0... BigDecimal is a strange beast :/

[cowtowncoder]

Right, I assumed this was the reason for inequality, and should result in retaining of the representation.
But since I hadn't verified thought better to play it safe. I vaguely recalled that there are typically methods for normalizing floating point numbers to canonical representation; but this is done automatically for doubles (when parsing).

[fge]

Out of curiosity, what do you mean by "[...]I hadn't verified"?

I haven't looked at the code for float or double yet, but if we refer to one book we seem to have in common ("Effective Java"), even for float or double, Josh Bloch recommends that Float.compareTo() or Double.compareTo() be used to test for equality since they will do the "right thing" for NaNs (Item 8 IIRC).

What I am pretty sure of, however, is that if the constructor of DecimalNode uses .stripTrailingZeroes(), .equals() will work. This is a one-liner change, except if a further feature is involved, of course.

[cowtowncoder]

Just meant I had not tried out that piece of code.

But as I said, I can see some users being unhappy if what comes out is different than what goes in. This based on history of changes that should be safe, but left to others being unhappy.

I also would not expect fix itself to be hard, just as you point out.

[fge]

OK, some background: I use JSON Schema a lot, and use Jackson as my core library. One of my core requirements is that I can validate numeric instances whatever their scale and/or precision, which is why I use the aforementioned feature, and resort to using BigDecimal in the following situations:

• the JSON value is a number with a fractional and/or an exponent part (JsonToken.VALUE_NUMBER_FLOAT); or
• it is an integer (JsonToken.VALUE_NUMBER_INT) but it does not fit into a long (which is why I asked, at the time, for .canConvertTo{Int,Long}()).

Now, as I said, if we go that far into equality, then should 1e-1 equal 0.1? BigDecimal says yes, however their JSON representation differs.

So, in any case, BigDecimal does not really fit the bill of "the JSON representation differs even though the mathematical representation is the same". And I don't think people will be this asstight, will they?

[fge]

Mishap, sorry... Didn't mean to close it

[cowtowncoder]

I agree in that intuitive equality is the better choice. The concern about representation changing is merely speculation; but when a jackson upgrade changes things like representation, unit tests can break, and to whomever this happens is usually unhappy at least initially.

But how about this: I'll happily merge a diff where JsonNodeFactory can be configured to not normalize value; but default being it does call it, Anyone who wants to customize this can then either disable normalization; or even override factory method in question for whatever logics is needed.

[fge]

Hmm, I forked jackson-databind, and when looking at JsonNodeFactory, currently, it does not have access to any feature set :/

And DecimalNode has no link to any feature set either. Which means such a change would be rather intrusive. Or am I looking in the wrong place?

[cowtowncoder]

Right, that's why I suggested in last comment that let's just configure it directly for JsonNodeFactory -- it unfortunately does not have access to features, and I dont think this warrants bigger refactoring.
So, just having on/off thing for JsonNodeFactory itself would be fine. If one day there'll be more things to tweak, I can figure out how to pipe general configuration, if that makes sense.

[fge]

You mean, as simple a diff as:

diff --git a/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java b/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
index 61fb078..a6eab2d 100644
--- a/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
+++ b/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
@@ -152,7 +152,7 @@ public class JsonNodeFactory
      * Factory method for getting an instance of JSON numeric value
      * that expresses given unlimited precision floating point value
      */
-    public NumericNode numberNode(BigDecimal v) { return DecimalNode.valueOf(v); }
+    public NumericNode numberNode(BigDecimal v) { return DecimalNode.valueOf (v.stripTrailingZeros()); }

     /*
     /**********************************************************

?

[cowtowncoder]

Possibly, although it'd be nice to have a boolean config setting (_cfgNormalizeBigDecimals or whatever), passed in ctor, defaulting to 'true', to be able to enable/disable behavior?

[fge]

This? (OK, this reflects my own coding preferences)

diff --git a/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java b/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
index 61fb078..d73849f 100644
--- a/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
+++ b/src/main/java/com/fasterxml/jackson/databind/node/JsonNodeFactory.java
@@ -12,14 +12,48 @@ import java.math.BigInteger;
  */
 public class JsonNodeFactory
 {
+    /*
+     * Should {@link DecimalNode} instances be built with exact representations
+     * of {@link BigDecimal} instances?
+     *
+     * This has quite an influence since, for instance, a BigDecimal constructed
+     * with string value "1.0" and another constructed with string value "1.00"
+     * will *not* be equal, since their scale differs (1 in the first case, 2 in
+     * the second case).
+     *
+     * By default, this is set to false, which means all DecimalNode instances
+     * will see their argument value "normalized" by calling {@link
+     * BigDecimal#stripTrailingZeros()}.
+     *
+     * Note that setting this to true (by calling {@link
+     * #withExactBigDecimals(boolean)} with true as an argument) does not
+     * guarantee a strict inequality between JSON representations: JSON
+     * values 0.1 and 1e-1 yield two equivalent BigDecimal instances since they
+     * have the same scale (1).</p>
+     */
+    private final boolean _cfgBigDecimalExact;
+
+    private static final JsonNodeFactory decimalsNormalized
+        = new JsonNodeFactory(false);
+    private static final JsonNodeFactory decimalsAsIs
+        = new JsonNodeFactory(true);
+
     /**
      * Default singleton instance that construct "standard" node instances:
      * given that this class is stateless, a globally shared singleton

      * can be used.
      */
-    public final static JsonNodeFactory instance = new JsonNodeFactory();
+    public final static JsonNodeFactory instance = decimalsNormalized;

-    protected JsonNodeFactory() { }
+    protected JsonNodeFactory(final boolean bigDecimalExact)
+    {
+        _cfgBigDecimalExact = bigDecimalExact;
+    }
+
+    public static JsonNodeFactory withExactBigDecimals(final boolean bigDecimalExact)
+    {
+        return bigDecimalExact ? decimalsAsIs : decimalsNormalized;
+    }

     /*
     /**********************************************************
@@ -152,7 +186,12 @@ public class JsonNodeFactory
      * Factory method for getting an instance of JSON numeric value
      * that expresses given unlimited precision floating point value
      */
-    public NumericNode numberNode(BigDecimal v) { return DecimalNode.valueOf(v); }
+    public NumericNode numberNode(BigDecimal v)
+    {
+        final BigDecimal d = _cfgBigDecimalExact ? v : v.stripTrailingZeros();
+
+        return DecimalNode.valueOf(d);
+    }

     /*
     /**********************************************************