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 2020/04/20 03:47:43 UTC
[GitHub] [tinkerpop] heljoyLiu opened a new pull request #1280: gremlin-python: fix up serializer out of range of int
heljoyLiu opened a new pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280
python integer support 64bit, but java integer only 32bit in server, so server may throw deserializer error when get a big integer value from `python sdk`.
```
org.apache.tinkerpop.shaded.jackson.databind.JsonMappingException: Could not deserialize the JSON value as required. Nested exception: org.apache.tinkerpop.shaded.jackson.core.JsonParseException: Numeric value (851401972585122) out of range of int
at [Source: (byte[])"{"op": "bytecode", "requestId": {"@type": "g:UUID", "@value": "a3660cfd-a531-4b92-8a0d-f65388675f63"}, "args": {"gremlin": {"@type": "g:Bytecode", "@value": {"step": [["V"], ["has", "customer_id", {"@type": "g:Int32", "@value": 851401972585122}]]}}, "aliases": {"g": "g"}}, "processor": "traversal"}"; line: 1, column: 244]
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on issue #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-616514375
issue [2360](https://issues.apache.org/jira/browse/TINKERPOP-2360) here
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1280: TINKERPOP-2360 gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-619941045
Travis seems to be passing tests now and this change seems like a good one. I'm not sure why we didn't have it this way before. We were already coercing `long()` to `BigInt` so I'm not sure why we wouldn't do the same for int. It makes me wonder if it's an inconsistency to not have an explicit `bigint()` function. I realize that Python developers don't think of numbers the way that Java developers do but if a Java server process explicitly required a `BigInt` and the user needed to send the number `1` of that type they wouldn't have a way to do it. There is no need for you to resolve that issue on this ticket, but perhaps it is worth creating a JIRA for it. Perhaps it would only be a feature for {{master}} and 3.5.0.
Finally, a couple of administrative points:
1. It would be nice if you could include a [CHANGELOG entry](https://github.com/apache/tinkerpop/blob/3.3-dev/CHANGELOG.asciidoc#tinkerpop-3311-release-date-not-officially-released-yet) that describes your change a bit. While we add JIRA ticket references to this file on release, we like to include short summaries of each change that goes in.
2. Could you please rebase and squash these commits after the changes I suggested are resolved.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on pull request #1280: TINKERPOP-2360 gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-620336469
> Python developers don't think of numbers the way that Java developers do but if a Java server process explicitly required a `BigInt` and the user needed to send the number `1` of that type they wouldn't have a way to do it.
which scenes do Java server explicitly require `1` of `BigInt`, I don't catch it. In server side, numbers type (integer, long, double ...) should be comparable
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-616508715
Actually, after writing all that feedback note that the python tests are now failing as a result of this change:
https://travis-ci.org/github/apache/tinkerpop/jobs/677088077
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on issue #1280: TINKERPOP-2360 gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-618159461
how to request `travis-ci` again, it's ok now. @spmallette
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#discussion_r411321142
##########
File path: gremlin-python/src/main/jython/tests/driver/test_client.py
##########
@@ -129,6 +129,17 @@ def test_multi_conn_pool(client):
assert len(result_set.all().result()) == 6
+def test_client_bytecode_with_int(client):
+ g = Graph().traversal()
+ t = g.V().has('age', 851401972585122).count()
Review comment:
Thus far we've forced python users to be explicit and think in java terms, thus:
```python
g.V().has('age', long(851401972585122)).count()
```
but I suppose it's better if this were just handled automatically as it might yield less confusion and be more pythonic. The test you added here is fine but please add a test here:
https://github.com/apache/tinkerpop/blob/3.3-dev/gremlin-python/src/main/jython/tests/structure/io/test_graphsonV2d0.py#L306
and here:
https://github.com/apache/tinkerpop/blob/3.3-dev/gremlin-python/src/main/jython/tests/structure/io/test_graphsonV3d0.py#L357
to more directly test GraphSON. Please do not remove the existing tests that use the old `long()` syntax as I think it's still worth testing that since it's something we will continue to support. Just add new assertions for your new logic. I guess it wouldn't hurt to add tests to cover both positive and negative values in your range.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on issue #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on issue #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-616513687
> Actually, after writing all that feedback note that the python tests are now failing as a result of this change:
>
> https://travis-ci.org/github/apache/tinkerpop/jobs/677088077
yes, I checked this error logs, and I miss a sense, sorry.
```
{"@type":"g:Int32","@value":true}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1280: TINKERPOP-2360 gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-620526498
Not all graphs coerce types so users may need to be explicit about the type they send. For example, if you want a specific number type like `BigInt` explicitly stored in TinkerGraph, you need to pass it to `property()` as that type. That said, Gremlin, the language, will make that underlying number comparable during a query.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] heljoyLiu commented on a change in pull request #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
heljoyLiu commented on a change in pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#discussion_r411329807
##########
File path: gremlin-python/src/main/jython/tests/driver/test_client.py
##########
@@ -129,6 +129,17 @@ def test_multi_conn_pool(client):
assert len(result_set.all().result()) == 6
+def test_client_bytecode_with_int(client):
+ g = Graph().traversal()
+ t = g.V().has('age', 851401972585122).count()
Review comment:
en, it's about serialization, I will move UT
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1280: gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on issue #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#issuecomment-616500676
Thanks for contributing again. Could you please create a JIRA to track this issue:
https://issues.apache.org/jira/projects/TINKERPOP/issues
we didn't do it for your last series of pull requests and I regretted that a bit afterwards.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tinkerpop] spmallette commented on a change in pull request #1280: TINKERPOP-2360 gremlin-python: fix up serializer out of range of int
Posted by GitBox <gi...@apache.org>.
spmallette commented on a change in pull request #1280:
URL: https://github.com/apache/tinkerpop/pull/1280#discussion_r415746197
##########
File path: gremlin-python/src/main/jython/tests/structure/io/test_graphsonV2d0.py
##########
@@ -307,6 +307,7 @@ def test_numbers(self):
assert {"@type": "gx:Byte", "@value": 1} == json.loads(self.graphson_writer.writeObject(int.__new__(SingleByte, 1)))
assert {"@type": "g:Int64", "@value": 2} == json.loads(self.graphson_writer.writeObject(long(2)))
assert {"@type": "g:Int32", "@value": 1} == json.loads(self.graphson_writer.writeObject(1))
+ assert {"@type": "g:Int64", "@value": 851401972585122} == json.loads(self.graphson_writer.writeObject(851401972585122))
Review comment:
Could we please take this opportunity to improve our unit testing in general:
1. Add tests for the negative ranges in the int/long/bigint serializers.
2. We validate `long(2)` which is in the range of int and coerces to `Int64` but could you please add a test for something like `long(851401972585122)` which is in the range of long to cover that 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org