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/06/30 22:01:18 UTC
[GitHub] [tinkerpop] simonz-bq opened a new pull request, #1743: Cast Times serialization to int
simonz-bq opened a new pull request, #1743:
URL: https://github.com/apache/tinkerpop/pull/1743
Gremlin server does not have the ability to take in arguments for `Times` as any type except for int. This ensures that if something can be interpreted to be serialized as an int from gremlin-go for `Times` that it is.
In Go, entering a number is considered as an `int`, which are serialized as a `long` due to language limitations. This allows people to use the `Times` step as expected instead of getting a server error and needing to know that the only resolution is being forced to specify the input as int32.
The only potential downside here is that if someone were to enter a number, which I would believe is inappropriate for the `Times` step anyways, that doesn't properly cast to int32 they may get an unexpected result.
--
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
[GitHub] [tinkerpop] simonz-bq commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
simonz-bq commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r914200413
##########
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:
I don't think this matters too much. Passing in a int32 cast is still a valid input, and the cucumber tests are meant to test input-output, not the semantics of how it's entered into gremlin-go. There are other examples of inputs not being exactly as you'd expect a real user to pass things in.
--
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
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r913169703
##########
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:
Yeah, main reason we are running into the issue is we needed to serialize go int into java long since it could be 64 bits. I'm more in favour of leaving letting the error be returned by the server instead of casting, and perhaps adding documentations so users know what to do in those case.
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r913154006
##########
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:
Then we might need to exclude also `int` and `uint`, right? I feel that would make this patch not very useful. If I'm not wrong, the common use case `.Times(123)` (integer literal) would not be covered anymore.
From the Go's spec:
```
There is also a set of predeclared integer types with implementation-specific sizes:
uint either 32 or 64 bits
int same size as uint
```
--
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
[GitHub] [tinkerpop] jroimartin commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1174042150
> I suppose it would involve checking within a range of bytecode steps that needs casting and perform the cast?
That's what I had in mind.
> Although in that case would we want to add this extra check for every bytecode request we make?
I guess it's a trade-off:
- Updating the GLV docs only: users might be confused at first, but it would be documented and it does not add tricky nuances to the API or extra over-head.
- Implementing it inside the `Times` step: it would cover some of the most common use cases, but bugs due to truncated integers can be very tricky and even more confusing.
- Implementing it inside `makeBytecodeRequest`: adds an overhead to every request. Although bytecode is already traversed to extract per-request arguments, so this could be maybe "merged" somehow to traverse the bytecode only once.
--
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
[GitHub] [tinkerpop] jroimartin commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1174026336
@simonz-bq @xiazcy would it be an option to deal with this kind of casts inside the function [makeBytecodeRequest](https://github.com/apache/tinkerpop/blob/40508a427756d088d38b2236837c09db5dd1f2cd/gremlin-go/driver/request.go#L66)?
It could be modified to return `(request, error)` and, then `(*Client).submitBytecode` could [return with error if it fails](https://github.com/apache/tinkerpop/blob/40508a427756d088d38b2236837c09db5dd1f2cd/gremlin-go/driver/client.go#L161). That would allow to check that the cast is valid before sending it to the server.
I'm asking because it seems that the root cause of the problem is dealing with this inside the step itself which cannot return an error.
--
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
[GitHub] [tinkerpop] xiazcy commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1174036128
> `makeBytecodeRequest` could be modified to return `(request, error)` and `(*Client).submitBytecode` could [return with error if it fails](https://github.com/apache/tinkerpop/blob/40508a427756d088d38b2236837c09db5dd1f2cd/gremlin-go/driver/client.go#L161). That would allow to check that the conversion is valid before sending it to the server.
>
@jroimartin That may work, and I suppose it would involve checking within a range of bytecode steps that needs casting and perform the cast? Although in that case would we want to add this extra check for every bytecode request we make?
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r911796155
##########
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` and `uint` are missing. Is it intended?
Other than that, I remember running into this problem when I started using gremlin-go and neither the solution nor the error were obvious. I ended up greping the repo to find a solution. So, in my opinion, this is a good change for people new to gremlin-go. However, I would update the `Times` documentation to mention that the passed value will be casted to `int32` and this could have unexpected results.
--
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
[GitHub] [tinkerpop] spmallette merged pull request #1743: Cast certain traversal step inputs as int32 for serialization in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
spmallette merged PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743
--
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
[GitHub] [tinkerpop] codecov-commenter commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1176539781
# [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#1743](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (05dedd5) into [3.5-dev](https://codecov.io/gh/apache/tinkerpop/commit/71f3ee0ab2fec3322b3a896222ad0ccbd6c0919c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (71f3ee0) will **decrease** coverage by `0.29%`.
> The diff coverage is `23.07%`.
```diff
@@ Coverage Diff @@
## 3.5-dev #1743 +/- ##
===========================================
- Coverage 63.51% 63.21% -0.30%
===========================================
Files 23 23
Lines 3601 3626 +25
===========================================
+ Hits 2287 2292 +5
- Misses 1136 1156 +20
Partials 178 178
```
| [Impacted Files](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [gremlin-go/driver/graphTraversal.go](https://codecov.io/gh/apache/tinkerpop/pull/1743/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z3JlbWxpbi1nby9kcml2ZXIvZ3JhcGhUcmF2ZXJzYWwuZ28=) | `85.64% <23.07%> (-4.26%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [40508a4...05dedd5](https://codecov.io/gh/apache/tinkerpop/pull/1743?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [tinkerpop] xiazcy commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
xiazcy commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r913216939
##########
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:
Also, the reason to suggest excluding `uint64` and `int64` is because I think we should enforce the fact that these steps only takes 32-bit integers or less, so we shouldn't allow users to consciously input types that is larger.
The ambiguity comes with `int`, as that is the default integer type for go if one doesn't specify the type, so that would be the main case we need to handle.
I'm not entirely sure how `uint` would be handled, since a 32-bit `uint` would technically fit to a long in java. Perhaps if we can figured out a good way to cast, this can be handled a similar way.
--
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
[GitHub] [tinkerpop] jroimartin commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1176808683
LGTM. Although, given my little knowledge of the project, mine are mostly suggestions and a way of learning more about the internals :slightly_smiling_face:
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r911796155
##########
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` and `uint` are missing. Is it intended?
Other than that, I remember running into this problem when I started using gremlin-go and neither the solution nor the error were obvious. I ended up greping the repo to find a solution. So, in my opinion, this is a good change for people new to gremlin-go. However, I would update the `Times` documentation to mention that the passed value will be casted to `int32`.
--
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
[GitHub] [tinkerpop] xiazcy commented on pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
xiazcy commented on PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#issuecomment-1174059978
> * Implementing it inside the `Times` step: it would cover some of the most common use cases, but bugs due to truncated integers can be very tricky and even more confusing.
Yeah, while it is unlikely for users to enter very large numbers, we probably should handle the case, and I don't see how we can do that here when we can't return errors in a traversal step.
> * Implementing it inside `makeBytecodeRequest`: adds an overhead to every request. Although bytecode is already traversed to extract per-request arguments, so this could be maybe "merged" somehow to traverse the bytecode only once.
I would agree this may be the most ideal if we can minimize the overhead, to make it more natural for users without the need to cast.
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r911796155
##########
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` and `uint` are missing. Is it intended?
Other than that, I remember running into this problem when I started using gremlin-go and neither the solution nor the error were obvious. I ended up greping the repo to find a solution. So, in my opinion, this is a good change for people new to gremlin-go. However, I would update the `Times` documentation to mention that the passed integer values will be casted to `int32` and this could have unexpected results.
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r913154006
##########
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:
Then we might need to exclude also `int` and `uint`, right? I feel that would make this patch not very useful. If I'm not wrong, the common use case `.Times(123)` would not be covered anymore.
From the Go's spec:
```
There is also a set of predeclared integer types with implementation-specific sizes:
uint either 32 or 64 bits
int same size as uint
```
--
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
[GitHub] [tinkerpop] jroimartin commented on a diff in pull request #1743: Cast Times serialization to int in Gremlin-Go
Posted by GitBox <gi...@apache.org>.
jroimartin commented on code in PR #1743:
URL: https://github.com/apache/tinkerpop/pull/1743#discussion_r914729329
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,38 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+const maxInt32 = 2147483647
Review Comment:
What about using `math.MaxInt32` instead?
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,38 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+const maxInt32 = 2147483647
+
+func int32Args(args ...interface{}) []interface{} {
+ for i, arg := range args {
+ switch val := arg.(type) {
+ case uint:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case uint32:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case uint64:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case int:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case int64:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
Review Comment:
Does it make sense to also check for `val >= math.MinInt32` in the case of `int` and `int64`? I guess the server would fail anyway, but being strict we could be truncating big negative numbers.
##########
gremlin-go/driver/graphTraversal.go:
##########
@@ -669,6 +670,38 @@ func (g *GraphTraversal) Write(args ...interface{}) *GraphTraversal {
return g
}
+const maxInt32 = 2147483647
+
+func int32Args(args ...interface{}) []interface{} {
+ for i, arg := range args {
+ switch val := arg.(type) {
+ case uint:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case uint32:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case uint64:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case int:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ case int64:
+ if val <= maxInt32 {
+ args[i] = int32(val)
+ }
+ default:
+ break
Review Comment:
I think the default case can be removed given that it is a no-op.
--
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