[
https://issues.apache.org/jira/browse/TINKERPOP-2992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18057468#comment-18057468
]
ASF GitHub Bot commented on TINKERPOP-2992:
-------------------------------------------
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.
> Gherkin suite missing orderability on set, uuid, date
> -----------------------------------------------------
>
> Key: TINKERPOP-2992
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2992
> Project: TinkerPop
> Issue Type: Improvement
> Components: test-suite
> Affects Versions: 3.6.5
> Reporter: Stephen Mallette
> Priority: Minor
>
> Orderability.feature has todos in it for set, uuid and date.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
