nelio2k
commented
4 years ago Based on some discussion about what is the right thing to do about NaN, as well as the complicated job of documenting gojsonsm behavior for the public when mixed type comparisons are used, I thought that it may be time to revisit the "validity" comparison argument.
The last commit push is to help make the matcher a bit more deterministic in its result. The idea is to mirror math.NaN comparisons that "if it doesn't make sense, don't return a deterministic true or false, but just fail the match". To do this, we'll just simply set binTreeState to Resolved, instead of true or false (which is subjected to NOT/NEOR ops).
With the proposed changes, the following unit tests need to be changed to align with the proposed behavior.
Test 1:
func TestMatcherMissingNotEquals(t *testing.T) {
// This tests a specific case where a missing value should actually
// result in a truthy result in the expression. Due to the nature
// of our bintree implementation, this needs special handling.
runJSONExprMatchTest(t, `
["not",
["equals",
["field", "sometimesValue"],
["value", "true"]
]
]
`, []string{
"5b47eb0950e9076fc0aecd52",
"5b47eb093771f06ced629663",
"5b47eb09ffac5a6ce37042e7",
"5b47eb095c3ad73b9925f7f8",
"5b47eb0962222a37d066e231",
"5b47eb09996a4154c35b2f98",
"5b47eb091f57571d3c3b1aa1",
"5b47eb098eee4b4c4330ec64",
})
}Document 5b47eb0950e9076fc0aecd52 originally was marked as expected to return “true” in the final output. However, the person document below actually does have “sometimesValue” as a key, and has a boolean as a type:
{
"_id": "5b47eb0950e9076fc0aecd52",
…
"sometimesValue": true,
}Previously, the matcher was returning “true” for this match because:
- It first takes the original expression of “true”
- It then tries to convert the boolean from the document, true, into a Json string for comparison.
- Fastval’s ToJsonString() returns an error, but compareStrings() did not check for such error.
- It then does a string comparison between “true” with a nil string, which is NOT equal, and thus this document match returned a “true” result.
Given the proposed change, this matcher is now returning a “false” for this match because:
- It first takes the original expression of “true”
- It then tries to convert the boolean from the document, true, into a Json string for comparison.
- This comparison is invalid because we cannot coerce a booleantlype into a string festival.
- The invalid op flag pops all the way up, and thus the match result is false.
Test 2:
func TestMatcherDisparateTypeEquals(t *testing.T) {
// TODO(brett19): Should probably discuss whether type-cast equals
// actually makes sense... This validates that these something like:
// (true == "thisShouldBeABoolean") === true
// which may not actually make a whole lot of sense...
runJSONExprMatchTest(t, `
["equals",
["field", "sometimesValue"],
["value", "thisShouldBeABoolean"]
]
`, []string{
"5b47eb0936ff92a567a0307e",
"5b47eb096b1d911c0b9492fb",
})
}Similar match logic as above. Comparing a string “thisShouldBeABoolean” to an actual boolean (true) in the documents doesn’t really make sense (as noted in the inline comment)
The proposed behavior is definitive: Gojsonsm will try to parse the data as a string to do string comparison… but it’s not possible to convert a boolean to a string. So the match behavior is undefined and will ensure that this match def does not pass.
As such, the above test is removed as a part of the PR.
brett19
This PR started off as to address NaN comparisons and then became larger and grander in scale. It now addresses how gojsonsm works with different data types, how implicit conversion (if there is a need) works, and also better defined collation as well. The documentation explaining the though process and details can be found here: https://docs.google.com/document/d/1aoN0eAS0S88-N3KYwHjVA5dW6IXXwW_KKNidQS5IG6I/edit?pli=1#heading=h.yqp9lhieqak3