iframe-proxy

erickj · 2023-09-14T09:43:55Z

Allows customizing the gremlin script engines traversal sources. This allows clients of the database to register tinkerpop traversal DSLs or customize the traversal strategies.

Motivation

Using tinkerpop DSLs and customizing traversal strategies.

Related issues

A list of issues either fixed, containing architectural discussions, otherwise relevant
for this Pull Request.

Additional Notes

Anything else we should know when reviewing?

Checklist

I have run the build using mvn clean package command (some tests seem flaky, but unrelated)
[x ] My unit tests cover both failure and success scenarios

customizing the gremlin script engines traversal sources. This allows clients of the database to register tinkerpop traversal DSLs or customize the traversal strategies.

erickj · 2023-09-14T09:46:18Z

validates GREMLIN_TRAVERSAL_BINDING entries conform to the expected Map<String, TraversalSupplier> form.

lvca

Thanks @erickj I think it's a cool feature to have integrated. Some questions in my review.

lvca · 2023-09-14T16:45:26Z

+                    </annotationProcessorPath>
+                  </annotationProcessorPaths>
+                  <annotationProcessors>
+                    <annotationProcessor>org.apache.tinkerpop.gremlin.process.traversal.dsl.GremlinDslProcessor</annotationProcessor>


What does the annotation processor do? Does it browse all the jars in the classpath looking for an annotation?

it builds the SocialTraversalSource from the SocialTraversalDsl for the tests:

https://tinkerpop.apache.org/docs/current/reference/#gremlin-java-dsl

I'm not familiar very familiar with maven, but I added the tinkerpop-annotation artifact as a test-scoped dependency only. Will that be enough to not affect the release jar? Can I limit the annotationprocessor to just the test?

test only = good.

…mote

lvca · 2023-09-14T21:17:17Z

Perfect now, merged. Thanks!

@erickj it would be awesome if you could add other developers to use the Gremlin DSL. Maybe a paragraph in our docs?
@gramian WDYT?

erickj · 2023-09-14T21:45:32Z

sg, I'll send a pull request on the docs repo in the next couple of days

lvca · 2023-09-14T22:02:33Z

Thanks

erickj · 2023-09-15T13:53:35Z

@lvca I'm sorry to create extra work on this, but unfortunately I think this needs to get rolled back in its current form and rethought. Currently it is entirely broken in the GremlinLangScriptEngine, which was overlooked in testing.

While working on writing up some documentation for this last night I was inspecting the Tinkerpop sources for the java GremlinLangScriptEngine, and came across a surprising & AFAICT undocumented check, which does not affect the groovy engine. The gremlin lang g4 grammar explicitly defines its TRAVERSAL_ROOT token as the literal 'g'. This is then validated in the GremlinLangeScriptEngine#eval and throws an error if the g graph is not bound.

I went back to verify this and tried adding another test to the GremlinTest make sure to set java as the GREMLIN_ENGINE, sure enough I get this parse error:

Caused by: javax.script.ScriptException: org.apache.tinkerpop.gremlin.language.grammar.GremlinParserException: Failed to interpret Gremlin query: Query parsing failed at line 1, character position at 0, error message : mismatched input 'friends' expecting {EmptyStringLiteral, 'g'}
	at org.apache.tinkerpop.gremlin.jsr223.GremlinLangScriptEngine.eval(GremlinLangScriptEngine.java:116)
	at java.scripting/javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:231)
	at com.arcadedb.gremlin.ArcadeGremlin.executeStatement(ArcadeGremlin.java:215)
	at com.arcadedb.gremlin.ArcadeGremlin.executeStatement(ArcadeGremlin.java:201)
	at com.arcadedb.gremlin.ArcadeGremlin.execute(ArcadeGremlin.java:67)

Worse yet, if I try to just rename the binding to g in the test I have realized that the entire Gremlin.g4 grammar seems to have no support for DSLs at all, as each traveral method is explicitly tokenized in the grammar.

I can't find any info on this issue in the tinkerpop issue tracker or mailing lists, so I'm asking on their answers server to see if there is more info.

I think in the meantime this can:

a.) be rolled back entirely
b.) only allowed for GroovScriptEngine

a.) is probably better IMO

lvca · 2023-09-15T14:33:27Z

@erickj no worries, glad you caught up before the release. Usually the Gremlin Discord channel is the best way to get help and they are super responsive. You could link this issue (they are familiar with ArcadeDB). For now maybe it's better to rollback waiting for a solution that works in both cases.

See #1239 (comment)

erickj · 2023-09-15T21:43:47Z

Just to wrap up on this, I've had a brief conversation w/ Stephen Mallette on the tinkerpop discord about this

https://discord.com/channels/838910279550238720/1152253500705755258

tldr; from his POV, there is no plan to support DSLs in the java interpreter at any point in the near future, and "the two ScriptEngine implementations are not meant to have complete feature parity. " Overall the expectation of DSL usage is to pre-translate the query to bytecode on a client endpoint via something like https://github.com/apache/tinkerpop/blob/master/gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/translator.js then send the bytecode over HTTP to the server endpoint (aside, personally, I've found this approach too heavyweight in the past as you need to carry around your DSL & Traversals in multiple languages and need a javascript interpreter & HTTP server somewhere in the chain)

In the end this will most likely only ever be supported by the groovy interpreter.

Overall it's a bit disappointing to hear. I'm not sure on your thoughts on the general usefulness of this, but I think for the direction of the arcade project it would be difficult for users to start to understand script engine specific features, and would be too leaky for any user who wasn't well versed in the issue.

@lvca lmkwyt

lvca · 2023-09-16T01:18:16Z

Thanks for the detailed explanation. So @erickj are you currently using it with the Groovy Gremlin engine? What's your experience with the DSL and ArcadeDB? When do you find useful and when does it work properly?

erickj · 2023-09-16T21:12:25Z

…rcadeData#1239) * adds global configuration key GREMLIN_TRAVERSAL_BINDINGS to allow customizing the gremlin script engines traversal sources. This allows clients of the database to register tinkerpop traversal DSLs or customize the traversal strategies. * lint cleanups * adds convenience function for creating TraversalSuppliers and validates GREMLIN_TRAVERSAL_BINDING entries conform to the expected Map<String, TraversalSupplier> form. * final fixup for typos, which I can't spot until after pushing to a remote * removes unused assignments in validation function * codacy style conformance

See ArcadeData#1239 (comment)

adds global configuration key GREMLIN_TRAVERSAL_BINDINGS to allow

db997f2

customizing the gremlin script engines traversal sources. This allows clients of the database to register tinkerpop traversal DSLs or customize the traversal strategies.

erickj added 2 commits September 14, 2023 14:59

lint cleanups

b2b84d9

adds convenience function for creating TraversalSuppliers and

00b8286

validates GREMLIN_TRAVERSAL_BINDING entries conform to the expected Map<String, TraversalSupplier> form.

lvca requested changes Sep 14, 2023

View reviewed changes

lvca self-assigned this Sep 14, 2023

lvca added the enhancement New feature or request label Sep 14, 2023

erickj added 3 commits September 14, 2023 18:55

final fixup for typos, which I can't spot until after pushing to a re…

925b0b4

…mote

removes unused assignments in validation function

95bef68

codacy style conformance

893c7c9

lvca added this to the 23.9.1 milestone Sep 14, 2023

lvca merged commit a547463 into ArcadeData:main Sep 14, 2023

lvca added a commit that referenced this pull request Sep 15, 2023

Removed recent merge waiting to resolve the issue from the Gremlin side

4126064

See #1239 (comment)

tae898 pushed a commit to humemai/arcadedb-embedded-python that referenced this pull request Jun 28, 2026

Removed recent merge waiting to resolve the issue from the Gremlin side

7d6d75c

See ArcadeData#1239 (comment)

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

erickj commented Sep 14, 2023

Motivation

Related issues

Additional Notes

Checklist

Uh oh!

erickj commented Sep 14, 2023

Uh oh!

lvca left a comment

Choose a reason for hiding this comment

Uh oh!

lvca Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

erickj Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lvca Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lvca commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erickj commented Sep 14, 2023

Uh oh!

lvca commented Sep 14, 2023

Uh oh!

erickj commented Sep 15, 2023

Uh oh!

lvca commented Sep 15, 2023

Uh oh!

erickj commented Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lvca commented Sep 16, 2023

Uh oh!

erickj commented Sep 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

erickj Sep 14, 2023 •

edited

Loading

lvca commented Sep 14, 2023 •

edited

Loading

erickj commented Sep 15, 2023 •

edited

Loading

erickj commented Sep 16, 2023 •

edited

Loading