Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DRIVERS-2651 Add decimal128 clamped zeros tests with very large exponents. #1432

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

matthewdale
Copy link
Contributor

@matthewdale matthewdale commented Jun 14, 2023

DRIVERS-2651

Add decimal128 Extended JSON parse tests for clamped zeros with very large positive and negative exponents to catch any issues in iterative clamping logic.

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded clusters, and serverless).

@matthewdale matthewdale requested a review from a team as a code owner June 14, 2023 18:56
@matthewdale matthewdale requested review from blink1073 and removed request for a team June 14, 2023 18:56
Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI Java driver fails all the new tests with an exception in the JDKs BigDecimal class: "Too many nonzero exponent digits (12)"

I think it's happening because it is unable to convert the $numberDecimal string value in degenerate_extjson into a Decimal128 instance.

@matthewdale
Copy link
Contributor Author

@jyemin that's really interesting! We could add these cases as expected parse errors instead of valid "degenerate_extjson". I'll check if there's an existing trend among drivers.

@jmikola
Copy link
Member

jmikola commented Jun 14, 2023

Created mongodb/mongo-php-driver#1433 to POC this using the PHP driver, which effectively tests libbson from the C driver.

There's no infinite loop (as mentioned in DRIVERS-2651), but I do see failures converting the degenerate extJSON to Canonical BSON. The PHP corpus tests exercise two code paths (one uses a BSON object as an intermediary representation) but this is really just one failure.

In the diffs below, the - segment is expected output and + is actual.


{
    "description": "clamped zeros with a large negative exponent",
    "canonical_bson": "180000001364000000000000000000000000000000000000",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-6176\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000000000
+180000001364000000000000000000000000000000fe5f00
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000000000
+180000001364000000000000000000000000000000fe5f00

{
    "description": "clamped zeros with a large positive exponent",
    "canonical_bson": "180000001364000000000000000000000000000000FE5F00",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E+6111\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000fe5f00
+180000001364000000000000000000000000000000000000
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000fe5f00
+180000001364000000000000000000000000000000000000

{
    "description": "clamped negative zeros with a large negative exponent",
    "canonical_bson": "180000001364000000000000000000000000000000008000",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-6176\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000008000
+180000001364000000000000000000000000000000fedf00
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000008000
+180000001364000000000000000000000000000000fedf00

{
    "description": "clamped negative zeros with a large positive exponent",
    "canonical_bson": "180000001364000000000000000000000000000000FEDF00",
    "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+999999999999\"}}",
    "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E+6111\"}}"
}
 // Degenerate extJSON -> Canonical BSON
-180000001364000000000000000000000000000000fedf00
+180000001364000000000000000000000000000000008000
 // Degenerate extJSON -> BSON object -> Canonical BSON
-180000001364000000000000000000000000000000fedf00
+180000001364000000000000000000000000000000008000

@matthewdale
Copy link
Contributor Author

@jyemin doing some quick investigation into that exception message, it seems possible that the Java BigDecimal parser will throw an exception if the exponent has more than 10 digits. I've updated the tests to use a number with exactly 10 digits, which should hopefully pass the Java tests now.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I'm getting:

Caused by: java.lang.NumberFormatException: Exponent overflow.
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:583)
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:471)
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:900)
	at org.bson.types.Decimal128.parse(Decimal128.java:131)
	at org.bson.json.JsonReader.visitNumberDecimalExtendedJson(JsonReader.java:1290)
	... 69 more

The BigDecimal code is:

                        if ((int) exp != exp) // overflow
                            throw new NumberFormatException("Exponent overflow.");

and exp = 9999999999 (0x2540BE3FF)

@matthewdale
Copy link
Contributor Author

@jyemin Hah, more validation! I've updated the test cases to use max/min int32 values for the exponents. It still triggers the problem in the old Go driver and hopefully passes the Java tests now.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Down from four to two failures now.

    {
      "description": "Clamped zeros with a large negative exponent",
      "canonical_bson": "180000001364000000000000000000000000000000000000",
      "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-2147483648\"}}",
      "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"0E-6176\"}}"
    }

    {
      "description": "Clamped negative zeros with a large negative exponent",
      "canonical_bson": "180000001364000000000000000000000000000000008000",
      "degenerate_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-2147483648\"}}",
      "canonical_extjson": "{\"d\" : {\"$numberDecimal\" : \"-0E-6176\"}}"
    }

They are similar, so here's the first:

Exception converting value '0E-2147483648' to type org.bson.types.Decimal128
org.bson.json.JsonParseException: Exception converting value '0E-2147483648' to type org.bson.types.Decimal128
	at app//org.bson.json.JsonReader.visitNumberDecimalExtendedJson(JsonReader.java:1292)
	at app//org.bson.json.JsonReader.visitExtendedJSON(JsonReader.java:670)
	at app//org.bson.json.JsonReader.readBsonType(JsonReader.java:166)
	at app//org.bson.codecs.BsonDocumentCodec.decode(BsonDocumentCodec.java:85)
	at app//org.bson.BsonDocument.parse(BsonDocument.java:66)
	at app//org.bson.GenericBsonTest.runValid(GenericBsonTest.java:144)
	at app//org.bson.GenericBsonTest.shouldPassAllOutcomes(GenericBsonTest.java:83)
Caused by: java.lang.NumberFormatException: Scale out of range.
	at java.base/java.math.BigDecimal.adjustScale(BigDecimal.java:705)
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:595)
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:471)
	at java.base/java.math.BigDecimal.<init>(BigDecimal.java:900)
	at org.bson.types.Decimal128.parse(Decimal128.java:131)
	at org.bson.json.JsonReader.visitNumberDecimalExtendedJson(JsonReader.java:1290)
	... 69 more

The check is:

    private int adjustScale(int scl, long exp) {
        long adjustedScale = scl - exp;
        if (adjustedScale > Integer.MAX_VALUE || adjustedScale < Integer.MIN_VALUE)
            throw new NumberFormatException("Scale out of range.");
        scl = (int) adjustedScale;
        return scl;
    }

When the exception is thrown:

scl = 0
exp = -2147483648 (0xFFFFFFFF80000000)
adjustedScale = 2147483648 (0x80000000)
Integer.MAX_VALUE = 0x7fffffff

So it might be enough to just bump the exponent by 1 so that `adjustedScale is equal to max int.

BigDecimal doc snippet:

The exponent consists of the character 'e' ('\u0065') or 'E' ('\u0045') followed by one or more decimal digits. The value of the exponent must lie between -Integer.MAX_VALUE (Integer.MIN_VALUE+1) and Integer.MAX_VALUE, inclusive.

@matthewdale
Copy link
Contributor Author

@jyemin good suggestion. I adjusted the negative exponent by +1.

Copy link
Contributor

@jyemin jyemin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green now for Java driver.

LGTM

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@matthewdale matthewdale merged commit c09f979 into mongodb:master Jul 4, 2023
@jmikola
Copy link
Member

jmikola commented Jul 13, 2023

From #1432 (comment)

I've updated the tests to use a number with exactly 10 digits, which should hopefully pass the Java tests now.

From #1432 (comment)

I've updated the test cases to use max/min int32 values for the exponents. It still triggers the problem in the old Go driver and hopefully passes the Java tests now.

@matthewdale: It looks like these changes resolved the previous failures I observed in libbson (via PHPC).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants