You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by GitBox <gi...@apache.org> on 2022/07/04 16:45:04 UTC
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
xiazcy commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r913134373
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -587,7 +587,8 @@ func (g *GraphTraversal) TimeLimit(args ...interface{}) *GraphTraversal {
// Times adds the times step to the GraphTraversal.
func (g *GraphTraversal) Times(args ...interface{}) *GraphTraversal {
- g.Bytecode.AddStep("times", args...)
+ // Gremlin server only accepts times step with integer argument values
+ g.Bytecode.AddStep("times", int32Args(args)...)
Review Comment:
Current cucumber test suite still uses int32() cast for the Times arguments (example [here](https://github.com/lyndonbauto/tinkerpop/blob/7ada477e466479d5378d72a476df6dd2b8a78941/gremlin-go/driver/cucumber/gremlin.go#L63)), the `GolangTranslator.java` will need to be updated at this [line](https://github.com/lyndonbauto/tinkerpop/blob/7ada477e466479d5378d72a476df6dd2b8a78941/gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/process/traversal/translator/GolangTranslator.java#L276) and `gremlin.go` file regenerated to reflect this change.
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,30 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+func int32Args(args ...interface{}) []interface{} {
+ for i, arg := range args {
+ switch val := arg.(type) {
+ case uint8:
+ args[i] = int32(val)
+ case uint32:
+ args[i] = int32(val)
+ case uint64:
Review Comment:
Should we include `uint64` and `int64`? Just in case someone enters a number larger than int32, casting would not give the correct result. I wonder if we should just propagates those types through, and let server error?
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,30 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+func int32Args(args ...interface{}) []interface{} {
+ for i, arg := range args {
+ switch val := arg.(type) {
+ case uint8:
+ args[i] = int32(val)
+ case uint32:
+ args[i] = int32(val)
+ case uint64:
+ args[i] = int32(val)
+ case int8:
+ args[i] = int32(val)
+ case int16:
+ args[i] = int32(val)
+ case int64:
+ args[i] = int32(val)
+ case int:
+ args[i] = int32(val)
Review Comment:
`uint16` is serialized as an `int32` so that was intentional. Not sure about `uint`.
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,30 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+func int32Args(args ...interface{}) []interface{} {
+ for i, arg := range args {
+ switch val := arg.(type) {
+ case uint8:
Review Comment:
`uint8`, `int8`, `int16` are not needed. Times() accept numbers shorter than integers.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org