Add GremlinLang to Go driver #2965

xiazcy · 2025-01-06T22:19:24Z

Base set of changes to add GremlinLang to Go driver, adapted from the originally translator. Datetime and strategies will be updated separately.
Additional clean-up and updates will be done once remote connection is fully set-up, tests will also remain disabled until then.

gremlin-go/driver/anonymousTraversal.go

andreachild · 2025-01-16T21:15:24Z

gremlin-go/driver/client.go

+// submitGremlinLang submits GremlinLang to the server to execute and returns a ResultSet.
+// TODO test and update when connection is set up
+func (client *Client) submitGremlinLang(gremlinLang *GremlinLang) (ResultSet, error) {
+	client.logHandler.logf(Debug, submitStartedBytecode, *gremlinLang)


The log message should be changed to gremlin lang

andreachild · 2025-01-16T21:24:19Z

gremlin-go/driver/driverRemoteConnection.go

+// TODO test and update when connection is set up
+func (driver *DriverRemoteConnection) submitGremlinLang(gremlinLang *GremlinLang) (ResultSet, error) {
+	if driver.isClosed {
+		return nil, newError(err0203SubmitBytecodeToClosedConnectionError)


Error message should be updated to reference gremlin lang.

andreachild · 2025-01-16T21:26:35Z

gremlin-go/driver/graphTraversal.go

@@ -922,7 +925,6 @@ func (t *Transaction) Begin() (*GraphTraversalSource, error) {

 	gts := &GraphTraversalSource{
 		graph:            t.g.graph,
-		bytecode:         t.g.bytecode,


Should this be replaced by gremlinLang: t.g.gremlinLang,?

andreachild · 2025-01-16T21:32:34Z

gremlin-go/driver/graphTraversalSource.go

@@ -42,95 +42,88 @@ func NewGraphTraversalSource(graph *Graph, remoteConnection *DriverRemoteConnect
 	convertedArgs := convertStrategyVarargs(traversalStrategies)
 	bc := NewBytecode(nil)


Should remove bc?

andreachild · 2025-01-16T21:44:31Z

gremlin-go/driver/graphTraversalSource.go

-		}
-	}
+	//TODO verify
+	var optionsStrategy TraversalStrategy = gts.gremlinLang.optionsStrategies[0]


Is it possible for optionStrategies to be empty and produce out of bounds error?

Yes, that's part of next step, which is to revisit how options is applied in go.

andreachild · 2025-01-16T21:55:25Z

gremlin-go/driver/gremlinlang.go

+)
+
+type GremlinLang struct {
+	emptyArray        []interface{}


Should this be [0]interface{} so that it cannot be mutated to be non-empty?

I'll keep this in mind and revisit. Current I've set it up where the method requires[]interface{} to be returned.

andreachild · 2025-01-16T22:13:53Z

gremlin-go/driver/gremlinlang.go

+
+func (gl *GremlinLang) addToGremlin(name string, args ...interface{}) {
+	flattenedArgs := gl.flattenArguments(args...)
+	//flattenedArgs := reflect.ValueOf(gl.flattenArguments(args...))


Should remove commented out code

andreachild · 2025-01-16T22:15:06Z

gremlin-go/driver/gremlinlang.go

+		gl.gremlin.WriteString("Cardinality.")
+		//str0, _ := gl.argAsString(flattenedArgs.Index(0))
+		//str1, _ := gl.argAsString(flattenedArgs.Index(1))
+		str0, _ := gl.argAsString(flattenedArgs[0])


Should there be error handling if the flattenedArgs are not an array of exactly 2 items as expected?

andreachild · 2025-01-16T23:05:43Z

gremlin-go/driver/gremlinlang.go

+	}
+}
+
+//var withOptionsMap map[any]string = map[any]string{


Should delete commented out code?

I'll be revisiting this. I'll add a TODO to remind me

andreachild · 2025-01-16T23:27:10Z

gremlin-go/driver/gremlinlang.go

+	switch v := arg.(type) {
+	case string:
+		// TODO check escaping & formatting
+		return fmt.Sprintf("\"%s\"", html.EscapeString(v)), nil


How about %q instead of %s + html.EscapeString(v) (since I don't think we want HTML syntax)?

https://pkg.go.dev/fmt#hdr-Printing

%q a double-quoted string safely escaped with Go syntax

I think a deeper dive is warranted here. The requirement is for it to be properly escaped according to gremlin-lang syntax (cannot close out the string literal and start writing subsequent steps). It's not immediately clear that Go or HTML escaping meets this requirement.

Yea, I think since we are concerned mainly with single and double quotes I'll just create a string replacer

andreachild · 2025-01-17T03:47:22Z

gremlin-go/driver/gremlinlang.go

+		return "null", nil
+	}
+
+	switch v := arg.(type) {


Do we need to worry about a few types not included in this switch statement such as complex64, complex128, uintptr?

I don't think we consider them, but I'll keep track of this and will come back to visit this point

andreachild · 2025-01-17T04:03:06Z

gremlin-go/driver/traversal.go

Should the line t.Bytecode.AddStep("discard") be converted to gremlin lang?

Cole-Greer · 2025-01-17T20:59:47Z

gremlin-go/driver/gremlinlang.go

+		return fmt.Sprintf("%s.%s", strings.ToUpper(name[:1])+name[1:], v), nil
+	case *Vertex:
+		id, _ := gl.argAsString(v.Id)
+		return fmt.Sprintf("new ReferenceVertex(%s,\"%s\")", id, v.Label), nil


There may be a need to escape the id and label here.

Cole-Greer · 2025-01-17T21:08:40Z

gremlin-go/driver/gremlinlang.go

+	return paramName
+}
+
+func (gl *GremlinLang) GetGremlin(arg ...string) string {


Nit: I wonder if we should limit using this vargargs pattern for psuedo-overloaded methods in favour of something more idiomatic in go. Feel free to ignore this request if you'd like as this pattern is already heavily used throughout gremlin-go.

xiazcy · 2025-01-27T23:21:32Z

VOTE +1

…tes will be needed when connection is set up.

Cole-Greer · 2025-01-27T23:57:19Z

VOTE +1

xiazcy · 2025-01-28T02:27:51Z

Merging to a feature branch instead. Will open PR to release branch for official review process once the go driver is fully complete.

andreachild reviewed Jan 16, 2025

View reviewed changes

gremlin-go/driver/anonymousTraversal.go Show resolved Hide resolved

andreachild reviewed Jan 16, 2025

View reviewed changes

andreachild reviewed Jan 17, 2025

View reviewed changes

gremlin-go/driver/traversal.go Outdated

Copy link

Contributor

andreachild Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the line t.Bytecode.AddStep("discard") be converted to gremlin lang?

Cole-Greer reviewed Jan 17, 2025

View reviewed changes

xiazcy added 3 commits January 27, 2025 15:23

Added GremlinLang to go driver to replace Bytecode. Clean up and upda…

9e986c8

…tes will be needed when connection is set up.

fix anon traversal

df703cf

update string escape and address comments to clean-up

f06e6a0

xiazcy force-pushed the go-gremlin-lang branch from 21974c6 to f06e6a0 Compare January 27, 2025 23:24

xiazcy changed the base branch from master to gremlin-go-http January 27, 2025 23:47

xiazcy merged commit 980ec24 into gremlin-go-http Jan 28, 2025
36 checks passed

xiazcy deleted the go-gremlin-lang branch January 28, 2025 02:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GremlinLang to Go driver #2965

Add GremlinLang to Go driver #2965

xiazcy commented Jan 6, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

xiazcy Jan 27, 2025

andreachild Jan 16, 2025

xiazcy Jan 27, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

andreachild Jan 16, 2025

xiazcy Jan 27, 2025

andreachild Jan 16, 2025

Cole-Greer Jan 17, 2025

xiazcy Jan 27, 2025

andreachild Jan 17, 2025

xiazcy Jan 27, 2025

andreachild Jan 17, 2025

Cole-Greer Jan 17, 2025

Cole-Greer Jan 17, 2025

xiazcy commented Jan 27, 2025

Cole-Greer commented Jan 27, 2025

xiazcy commented Jan 28, 2025

		@@ -42,95 +42,88 @@ func NewGraphTraversalSource(graph Graph, remoteConnection DriverRemoteConnect
		convertedArgs := convertStrategyVarargs(traversalStrategies)
		bc := NewBytecode(nil)

Add GremlinLang to Go driver #2965

Add GremlinLang to Go driver #2965

Conversation

xiazcy commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiazcy commented Jan 27, 2025

Cole-Greer commented Jan 27, 2025

xiazcy commented Jan 28, 2025