Replace Bytecode with GremlinLang in JavaScript GLV#3307
Replace Bytecode with GremlinLang in JavaScript GLV#3307Cole-Greer merged 2 commits intoapache:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3307 +/- ##
============================================
- Coverage 77.87% 75.82% -2.05%
+ Complexity 13578 13276 -302
============================================
Files 1015 1020 +5
Lines 59308 59864 +556
Branches 6835 7034 +199
============================================
- Hits 46184 45391 -793
- Misses 10817 11804 +987
- Partials 2307 2669 +362 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Cole-Greer
left a comment
There was a problem hiding this comment.
Overall I think this looks really good. Most of my comments are just for context and not anything which needs action in this PR, the rest are all for minor clarifications and tweaks.
...lin-javascript/src/main/javascript/gremlin-javascript/lib/driver/driver-remote-connection.ts
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/graph-traversal.ts
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/gremlin-lang.ts
Outdated
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/gremlin-lang.ts
Outdated
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/process/gremlin-lang.ts
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/gremlin-lang-test.js
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/gremlin-lang-test.js
Outdated
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js
Show resolved
Hide resolved
gremlin-javascript/src/main/javascript/gremlin-javascript/test/unit/traversal-test.js
Show resolved
Hide resolved
|
The overall design is similar to what has been done to the other GLVs for GremlinLang so my VOTE is +1, pending resolution of other comments. |
| const iso = arg.toISOString().replace('.000Z', 'Z'); | ||
| return `datetime("${iso}")`; | ||
| } | ||
| if (typeof arg === 'number') { |
There was a problem hiding this comment.
Given JS doesn't distinguish the number types, I wonder if we should be identifying the numerical types in gremlin by asserting their sizes for adding the suffix instead of just turning it to string directly?
There was a problem hiding this comment.
Numbers will be a giant mess here as there's no clear way to assume the users intent and map number into all of the TP numeric types. I expect we will need to accept some amount of precision loss and/or non-roundtripable types. My vote is to leave this for now and handle it separately. That topic alone can easily spawn many debates around handling subtle edge cases.
|
Overall I think this is a solid start for switching to GremlinLang in JS. There will be areas that needs follow-up changes, but I'm good to leave them out of this PR. VOTE +1 |
|
Thanks for the changes @kirill-stepanishin. LGTM VOTE +1 |
As part of the TP4 migration, the server no longer accepts Bytecode and only processes Gremlin queries in script form.. This PR replaces
Bytecode+Translatorwith a newGremlinLangclass.Changes
GremlinLangclass that incrementally builds Gremlin query strings as steps are addedGraphTraversalSource/GraphTraversalnow accumulate intoGremlinLanginstead ofBytecodeClient.submit()only accepts strings now; all queries sent asgremlin-langlanguage.OptionsStrategyextracted intoRequestOptions(timeout, batchSize, bulkResults, etc.)Bytecode,Translator,BytecodeSerializer, and their testsIntentionally out of scope for this PR