Hi all,
I have a couple of suggestions for the GLV.
Firstly, it would be useful to be able to inject arbitrary expressions, such as
static constructors. For example, in an application that I'm currently working
on, I use Titan's Geoshape property type. I would like to be able to call one
of the static constructors on the Geoshape class in a script, passing it bound
parameters. Something like
"g.V().has('location', geoWithin(Geoshape.circle(lat, lon, radius)))"
with bound parameters {'lat': 45, 'lon': 45, 'radius': '10'}
I took an initial stab at this today, and this is what I came up with:
https://gist.github.com/leifurhauks/5a843379183123dbed1134a92691a335
With those changes, I can use that static constructor in a traversal as follows:
>>> translator = GroovyTranslator('g')
>>> g = PythonGraphTraversalSource(translator)
>>> t = g.V().has('location', RawExpression('Geoshape.point(', B('lat', 45), ',
>>> ', B('lon', 45), ')'))
>>> str(t)
'g.V().has("location", Geoshape.point(lat, lon))'
>>> t.bindings
{'lon': 45, 'lat': 45}
That raw expression isn't very readable, but a simple helper class can fix that:
class Geoshape(object):
@staticmethod
def point(latitude, longitude, symbols=('lat', 'lon')):
return RawExpression(
'Geoshape.point(', B(symbols[0], latitude), ', ', B(symbols[1],
longitude), ')')
Now I can rewrite the previous traversal to be much clearer:
t = g.V().has('location', Geoshape.point(45, 45))
If this seems like a reasonable approach, I would be happy to submit a PR.
I have one other suggestion, but this one is tiny. Because most of the steps on
PythonGraphTraversal have the same implementation, it would be possible to
specify that implementation once in a function factory, like this:
def simple_step(name):
def step_method(self, *args):
self.translator.addStep(self, name, *args)
for arg in args:
if type(arg) is B:
self.bindings[arg.symbol] = arg.value
return self
return step_method
Then, on PythonGraphTraversal, all the step methods that use that
implementation could be declared like this:
class PythonGraphTraversal(object):
def __init__(self, translator, remote_connection=None):
# elided ...
# top methods elided
has = simple_step('has')
hasId = simple_step('hasId')
hasKey = smple_step('hasKey')
# and so on...
________________________________________
From: David Brown <[email protected]>
Sent: June 15, 2016 3:23 PM
To: [email protected]
Subject: Re: gremlin_python GLV
Ok Stephen I will. I can't claim to be an expert (especially when it
comes to the Java ecosystem), but I will definitely take a look.
Also, Marko, Python doesn't really have primitives as such. The built
in function isinstance should work for everything in gremlin_python
e.g. ``isinstance(var, bool)``. I can take a look at this when I make
a PR if you'd like, but it really isn't a huge deal anyway.
On Wed, Jun 15, 2016 at 3:16 PM, Stephen Mallette <[email protected]> wrote:
> David, it would also be great to get your feedback on the
> packaging/deployment approach we have so far. I have some basic figured out
> for deployment to pypi over maven via twine, but it could use an experts
> eye. You probably don't need to check that part out now as you are already
> have some other stuff to look at, but I just wanted to mention that so that
> it was in your mind for later. Thanks for your help on this.
>
> On Wed, Jun 15, 2016 at 3:02 PM, Marko Rodriguez <[email protected]>
> wrote:
>
>> Hello,
>>
>> > I was reading through the gremlin_python GLV code this morning, and
>> > overall it looks like it should work pretty smoothly. Thanks for doing
>> > this Marko, really cool work! I just wanted to comment on a few things
>> > that popped out at me on my first reading.The major issue I see is
>> > that it is not currently Python 2/3 compatible. This is due to two
>> > things:
>>
>> Cool! Thank you for taking your time to review the work.
>>
>> > 1. The way iterators are implemented in Python 2/3 is different. This
>> > problem could be remedied by adding a method to
>> > ``PythonGraphTraversal``, something like:
>> >
>> > def __next__(self):
>> > return self.next()
>> >
>> > then, in the ``next`` method, changing line 113 to
>> > ``next(self.results)``, should take care of this problem.
>>
>> Updated.
>>
>> > 2. The ``GroovyTranslator`` class checks for type ``long``, this no
>> > longer exists in Python 3. Determining what to submit as a Long might
>> > require some discussion, but this could be easily fixed by adding
>> > something like:
>> >
>> > import sys
>> > if sys.version_info.major > 2:
>> > long = int
>> >
>> > to the top of the file.
>>
>> Updated.
>>
>> >
>> > Other than this, there are some minor details that could be cleaned
>> > up. Particularly:
>> >
>> > 1. Using ``isinstance`` instead of ``type`` to perform type checking.
>> > This will recognize subclasses and is the most Pythonic way to do
>> > this.
>>
>> Seems isinstance() isn’t a method on primitives — only objects. Thus, the
>> code got complex. Left it with type().
>>
>> >
>> > 2. Formatting - indents, and line spacing. Typically, Python methods
>> > are separated by a single line, and indents use four spaces. This is
>> > really just cosmetic for readability.
>>
>> Uh. The problem is that the source code is auto-generated from
>> GremlinPythonGenerator. If you want to tweak, please do so:
>>
>>
>> https://github.com/apache/tinkerpop/blob/TINKERPOP-1278/gremlin-variant/src/main/groovy/org/apache/tinkerpop/gremlin/python/GremlinPythonGenerator.groovy
>> <
>> https://github.com/apache/tinkerpop/blob/TINKERPOP-1278/gremlin-variant/src/main/groovy/org/apache/tinkerpop/gremlin/python/GremlinPythonGenerator.groovy
>> >
>>
>>
>> > 3. CamelCase vs. underscores. I understand that to emulate Gremlin,
>> > the traversal methods etc. should use CamelCase. But I wonder if the
>> > helper classes (Translators) should use the Python convention of using
>> > underscores to name methods. Python class names use camel case by
>> > convention.
>>
>>
>> I haven’t changed it. If you feel we should, please do. And yes, I think
>> its good to keep the Gremlin step names camelCase. For Gremlin-Ruby, we
>> will do out_e() style.
>>
>> > Finally, the implementation of the B class may need some work, but
>> > we'll have to play around with it a bit to figure out how the best
>> > approach to doing this.
>>
>>
>> Yea, thats all wrong. I overdosed on the introspection and then Kuppitz
>> was like “Why not just have it as….” (something much simpler — which I
>> forget what it was now).
>>
>> > I'm sure there are more improvements, but I just wanted to get a
>> > conversation going. I would be happy to make a PR with some of these
>> > changes.
>>
>> That’d be awesome. Its in TINKERPOP-1278 branch. In gremlin-variant/test
>> you will see PythonProcessStandardTest and PythonProcessComputerTest. Those
>> verify that the compilation is valid for all the standard and computer
>> tests.
>>
>> > Also, gremlinclient will soon support the RemoteConnection interface,
>> > I'll send out a link to the docs once I get everything up and running.
>>
>> So cool. Please do review the Python RemoteConnection class as again, I
>> just improv’d it.
>>
>> Thanks again David,
>> Marko.
>>
>> http://markorodriguez.com
--
David M. Brown
R.A. CulturePlex Lab, Western University
