Cole-Greer commented on code in PR #3303: URL: https://github.com/apache/tinkerpop/pull/3303#discussion_r2785106381
########## gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/language/translator/JavascriptTranslateVisitor.java: ########## @@ -38,6 +38,8 @@ * <li>Makes anonymous traversals explicit with double underscore</li> * <li>Makes enums explicit with their proper name</li> * </ul> + * <p/> + * Assumes use of https://www.npmjs.com/package/uuid library for UUID handling. Review Comment: Looks like the uuid package supports modern browsers: https://www.npmjs.com/package/uuid#support I don't think this should be a concern. ########## gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/util/GremlinValueComparator.java: ########## @@ -241,7 +240,7 @@ public enum Type { Nulltype, Boolean (Boolean.class), Number (Number.class), - Date (Date.class), + Date (OffsetDateTime.class), Review Comment: It occurs to me that our ordering semantics don't extend beyond the set of serializable types for remote traversals. It's conceivable for embedded usage that a user may wish to order a mix of `OffsetDateTime`, `Date`, `ZonedDateTime`, `Year`, `Timestamp`... Theoretically those should all be comparable to each other. Currently GremlinValueComparator really isn't capable of handling comparisons between distinct types at the same "level", especially considering there is no single unifying interface for all "date-like" types in Java. This definitely isn't a new issue, or really all that applicable to this PR, but it may be an interesting consideration for the future. ########## gremlin-go/driver/cucumber/cucumberSteps_test.go: ########## @@ -588,174 +592,205 @@ func (tg *tinkerPopGraph) theResultShouldBe(characterizedAs string, table *godog } } +// compareMapEquals compares two maps for equality by checking if they have the same length +// and if every key-value pair in the actual map has a logically equal counterpart in the expected map. func compareMapEquals(expected map[interface{}]interface{}, actual map[interface{}]interface{}) bool { + if len(actual) != len(expected) { + return false + } for k, a := range actual { var e interface{} containsKey := false for ke, ee := range expected { - if fmt.Sprint(k) == fmt.Sprint(ke) { + if compareSingleEqual(k, ke) { containsKey = true e = ee break - } else { - if reflect.ValueOf(k).Kind() == reflect.Ptr && - reflect.ValueOf(ke).Kind() == reflect.Ptr { - switch k.(type) { - case *gremlingo.Vertex: - switch ke.(type) { - case *gremlingo.Vertex: - if fmt.Sprint(*k.(*gremlingo.Vertex)) == fmt.Sprint(*ke.(*gremlingo.Vertex)) { - containsKey = true - } - default: - // Not equal. - } - default: - // If we are here we probably need to implement an additional type like the Vertex above. - if fmt.Sprint(*k.(*interface{})) == fmt.Sprint(*ke.(*interface{})) { - fmt.Println("WARNING: Encountered unknown pointer type as map key.") - containsKey = true - } - } - if containsKey { - e = ee - break - } - } } } if !containsKey { fmt.Printf("Map comparison error: Failed to find key %s in %v\n", k, expected) return false } - if a == nil && e == nil { - continue - } else if a == nil || e == nil { - // One value is nil, other is not. They are not equal. - fmt.Printf("Map comparison error: One map has a nil key, other does not.\n") + if !compareSingleEqual(a, e) { + fmt.Printf("Map comparison error: Expected != Actual (%v!=%v)\n", e, a) return false - } else { - switch reflect.TypeOf(a).Kind() { - case reflect.Array, reflect.Slice: - switch reflect.TypeOf(e).Kind() { - case reflect.Array, reflect.Slice: - // Compare arrays - if !compareListEqualsWithoutOrder(e.([]interface{}), a.([]interface{})) { - return false - } - default: - fmt.Printf("Map comparison error: Expected type is Array/Slice, actual is %s.\n", reflect.TypeOf(a).Kind()) - return false - } - case reflect.Map: - switch reflect.TypeOf(a).Kind() { - case reflect.Map: - // Compare maps - if !compareMapEquals(e.(map[interface{}]interface{}), a.(map[interface{}]interface{})) { - return false - } - default: - fmt.Printf("Map comparison error: Expected type is Map, actual is %s.\n", reflect.TypeOf(a).Kind()) - return false - } - default: - if fmt.Sprint(a) != fmt.Sprint(e) { - fmt.Printf("Map comparison error: Expected != Actual (%s!=%s)\n", fmt.Sprint(a), fmt.Sprint(e)) - return false - } - } } } return true } -func compareListEqualsWithoutOrder(expected []interface{}, actual []interface{}) bool { - // This is a little weird, but there isn't a good solution to either of these problems: - // 1. Comparison of types in Go. No deep equals which actually works properly. Needs to be done manually. - // 2. In place deletion in a loop. - // So to do in place deletion in a loop we can do the following: - // 1. Loop from back to wrong (don't need to worry about deleted indices that way. - // 2. Create a new slice with the index removed when we fix the item we want to delete. - // To do an orderless copy, a copy of the expected result is created. Results are removed as they are found. This stops - // the following from returning equal [1 2 2 2] and [1 1 1 2] - - // Shortcut. - if fmt.Sprint(expected) == fmt.Sprint(actual) { +// compareSingleEqual is the core logical equality function used for Gremlin Gherkin tests. +// It avoids using fmt.Sprint which can include memory addresses for pointers (like Vertices or Edges), +// leading to false negatives in tests. It performs recursive comparisons for complex types +// and handles Gremlin-specific elements by comparing their IDs. +func compareSingleEqual(actual interface{}, expected interface{}) bool { + if actual == nil && expected == nil { return true + } else if actual == nil || expected == nil { + return false } + + actualSet, ok1 := actual.(*gremlingo.SimpleSet) + expectedSet, ok2 := expected.(*gremlingo.SimpleSet) + if ok1 && ok2 { + return compareListEqualsWithoutOrder(expectedSet.ToSlice(), actualSet.ToSlice()) + } else if ok1 || ok2 { + return false + } + + actualPath, ok1 := actual.(*gremlingo.Path) + expectedPath, ok2 := expected.(*gremlingo.Path) + if ok1 && ok2 { + return compareListEqualsWithOrder(expectedPath.Objects, actualPath.Objects) + } else if ok1 || ok2 { + return false + } + + actualV, ok1 := actual.(*gremlingo.Vertex) + expectedV, ok2 := expected.(*gremlingo.Vertex) + if ok1 && ok2 { + return actualV.Id == expectedV.Id + } + + actualE, ok1 := actual.(*gremlingo.Edge) + expectedE, ok2 := expected.(*gremlingo.Edge) + if ok1 && ok2 { + return actualE.Id == expectedE.Id + } + + actualVP, ok1 := actual.(*gremlingo.VertexProperty) + expectedVP, ok2 := expected.(*gremlingo.VertexProperty) + if ok1 && ok2 { + return actualVP.Id == expectedVP.Id + } + + actualP, ok1 := actual.(*gremlingo.Property) + expectedP, ok2 := expected.(*gremlingo.Property) + if ok1 && ok2 { + return actualP.Key == expectedP.Key && compareSingleEqual(actualP.Value, expectedP.Value) + } + + switch a := actual.(type) { + case map[interface{}]interface{}: + e, ok := expected.(map[interface{}]interface{}) + if !ok { + return false + } + return compareMapEquals(e, a) + case []interface{}: + e, ok := expected.([]interface{}) + if !ok { + return false + } + return compareListEqualsWithOrder(e, a) + case int, int8, int16, int32, int64, uint, uint8, uint16, uint32, uint64, float32, float64: + return compareNumeric(actual, expected) + default: + return fmt.Sprint(actual) == fmt.Sprint(expected) + } +} + +// compareNumeric normalizes different numeric types to float64 to allow for comparison +// between different precisions and representations (e.g. int64 vs float32). +// It also explicitly handles NaN equality which is required for some Gremlin tests. +func compareNumeric(actual interface{}, expected interface{}) bool { + var aVal, eVal float64 + switch a := actual.(type) { + case int: + aVal = float64(a) + case int8: + aVal = float64(a) + case int16: + aVal = float64(a) + case int32: + aVal = float64(a) + case int64: + aVal = float64(a) Review Comment: Nit: this conversion can be lossy, especially for values with large magnitudes. It appears we are getting away with it for now, perhaps that's good enough for the moment. I'm not sure where all of the cross type numeric comparisons are coming from in our tests, perhaps we could get away with normalizing integer types to int64 and float types to float64 without breaking any tests. This would solve the precision issue at the expense of disallowing integer to float comparisons. We could also add logic to normalize to the smallest common type if these conversions ever prove troublesome for future tests. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
