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/02 18:29:54 UTC
[GitHub] [tinkerpop] vkagamlyk opened a new pull request, #1676: Valentyn/an 1165 python graphbinary (#121)
vkagamlyk opened a new pull request, #1676:
URL: https://github.com/apache/tinkerpop/pull/1676
Finish graphbinary support in gremlin-python
https://issues.apache.org/jira/browse/TINKERPOP-2693
--
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] lyndonbauto merged pull request #1676: TINKERPOP-2693 Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto merged PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676
--
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] lyndonbauto commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889198146
##########
gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py:
##########
@@ -59,7 +59,7 @@ def __init__(self, url, traversal_source="g", protocol_factory=None,
# so that they can be closed if this parent session is closed.
self.__spawned_sessions = []
- if message_serializer is None:
+ if message_serializer is None and graphson_reader is not None and graphson_writer is not None:
Review Comment:
Did you encounter a case where graphson_read/graphson_writer are None?
Just wondering why this is required here but not in client.py
--
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] lyndonbauto commented on pull request #1676: TINKERPOP-2693 Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#issuecomment-1147642180
LGTM VOTE+1
--
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] vkagamlyk commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889226039
##########
gremlin-python/src/main/python/gremlin_python/structure/io/graphbinaryV1.py:
##########
@@ -255,15 +257,15 @@ class LongIO(_GraphBinaryTypeIO):
@classmethod
def dictify(cls, obj, writer, to_extend, as_value=False, nullable=True):
if obj < -9223372036854775808 or obj > 9223372036854775807:
- raise Exception("TODO: don't forget bigint")
+ raise Exception("Value too big, please use bigint Gremlin type")
Review Comment:
implicit conversion can introduce new issues, for example
`g.inject(1).repeat(__.addV('person').property('name', __.loops())).times(9223372036854775807 + 1).count()`
leads to server exception
`[ERROR] TraversalOpProcessor - Could not deserialize the Traversal instance
java.lang.IllegalStateException: null: .times(BigInteger)`
if you think that this is not an issue, then I will add implicit conversion
--
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] lyndonbauto commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889202740
##########
gremlin-python/src/main/python/tests/driver/test_client.py:
##########
@@ -27,10 +27,13 @@
from gremlin_python.process.strategies import OptionsStrategy
from gremlin_python.structure.graph import Graph
from gremlin_python.driver.aiohttp.transport import AiohttpTransport
+from gremlin_python.statics import *
Review Comment:
I see the other test just imports * already. Perhaps we can ignore this comment since the other tests don't consider that.
--
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] vkagamlyk commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889228829
##########
gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py:
##########
@@ -59,7 +59,7 @@ def __init__(self, url, traversal_source="g", protocol_factory=None,
# so that they can be closed if this parent session is closed.
self.__spawned_sessions = []
- if message_serializer is None:
+ if message_serializer is None and graphson_reader is not None and graphson_writer is not None:
Review Comment:
this code is related to setting default serializer
--
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] lyndonbauto commented on a diff in pull request #1676: TINKERPOP-2693 Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r890323714
##########
gremlin-python/src/main/python/gremlin_python/structure/io/graphbinaryV1.py:
##########
@@ -255,15 +257,15 @@ class LongIO(_GraphBinaryTypeIO):
@classmethod
def dictify(cls, obj, writer, to_extend, as_value=False, nullable=True):
if obj < -9223372036854775808 or obj > 9223372036854775807:
- raise Exception("TODO: don't forget bigint")
+ raise Exception("Value too big, please use bigint Gremlin type")
Review Comment:
Spoke with both, both agreed with your technique.
--
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] lyndonbauto commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889202141
##########
gremlin-python/src/main/python/tests/driver/test_client.py:
##########
@@ -27,10 +27,13 @@
from gremlin_python.process.strategies import OptionsStrategy
from gremlin_python.structure.graph import Graph
from gremlin_python.driver.aiohttp.transport import AiohttpTransport
+from gremlin_python.statics import *
Review Comment:
best practice is to import specific items
```suggestion
from gremlin_python.statics import bigint, long
```
--
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] lyndonbauto commented on pull request #1676: TINKERPOP-2693 Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#issuecomment-1151473515
Going to merge on lazy consensus.
--
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] lyndonbauto commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889201357
##########
gremlin-python/src/main/python/gremlin_python/structure/io/graphbinaryV1.py:
##########
@@ -255,15 +257,15 @@ class LongIO(_GraphBinaryTypeIO):
@classmethod
def dictify(cls, obj, writer, to_extend, as_value=False, nullable=True):
if obj < -9223372036854775808 or obj > 9223372036854775807:
- raise Exception("TODO: don't forget bigint")
+ raise Exception("Value too big, please use bigint Gremlin type")
Review Comment:
Should we just promote the type to bigint here? That way we handle this implictly and explicitly. Meaning you could do something like:
`g.V().....property("bigint_explicit", bigint(1)).property("bigint_implicit", 9223372036854775807 + 1)`.
--
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] vkagamlyk commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
vkagamlyk commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889240550
##########
gremlin-python/src/main/python/gremlin_python/driver/driver_remote_connection.py:
##########
@@ -59,7 +59,7 @@ def __init__(self, url, traversal_source="g", protocol_factory=None,
# so that they can be closed if this parent session is closed.
self.__spawned_sessions = []
- if message_serializer is None:
+ if message_serializer is None and graphson_reader is not None and graphson_writer is not None:
Review Comment:
reverted in https://github.com/apache/tinkerpop/pull/1676/commits/ff4e16c18236af61491e680cbf810664aba38e56
--
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] lyndonbauto commented on a diff in pull request #1676: Finish graphbinary support in gremlin-python
Posted by GitBox <gi...@apache.org>.
lyndonbauto commented on code in PR #1676:
URL: https://github.com/apache/tinkerpop/pull/1676#discussion_r889235996
##########
gremlin-python/src/main/python/gremlin_python/structure/io/graphbinaryV1.py:
##########
@@ -255,15 +257,15 @@ class LongIO(_GraphBinaryTypeIO):
@classmethod
def dictify(cls, obj, writer, to_extend, as_value=False, nullable=True):
if obj < -9223372036854775808 or obj > 9223372036854775807:
- raise Exception("TODO: don't forget bigint")
+ raise Exception("Value too big, please use bigint Gremlin type")
Review Comment:
Fair point. I am not sure myself I will tag some people who might know - @krlawrence @spmallette any thoughts?
--
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