LinkedQL: Context step

[iddan]

Depends on #902

• Define context step
• Add voc.Namespaces to all steps path building
• Separate steps definition to easily find where IRIs need to be parsed
• Removed Type() method to reduce quad.IRI references
• Parse all IRIs using the newly provided ns

---

This change is 

[dennwc]

Reviewed 7 of 7 files at r1, 9 of 48 files at r2, 43 of 43 files at r3, 3 of 3 files at r4, 1 of 1 files at r5, 1 of 1 files at r6, 14 of 14 files at r7, 1 of 1 files at r8, 2 of 2 files at r9, 2 of 2 files at r10.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @iddan)

---

internal/linkedql/schema/schema.go, line 27 at r8 (raw file):

func typeToRange(t reflect.Type) string {
	if t == stringMap {

Probably worth changing to t.Kind() == reflect.Map.

---

internal/linkedql/schema/schema.go, line 28 at r8 (raw file):

func typeToRange(t reflect.Type) string {
	if t == stringMap {
		return "rdf:JSON"

Should use voc.Prefix. Also, are you sure it's defined there?

---

query/linkedql/entity_identfier.go, line 17 at r7 (raw file):

// BuildIdentifier implements EntityIdentifier
func (i EntityIRI) BuildIdentifier(ns *voc.Namespaces) (quad.Value, error) {
	return AbsoluteIRI(quad.IRI(i), ns), nil

Nit: i -> e/p/iri

---

query/linkedql/iter_tags.go, line 16 at r2 (raw file):

// or all the tags in the query.
type TagsIterator struct {
	ValueIt  *ValueIterator

Why do you export those? To make a separate steps package work.

---

query/linkedql/linkedql.go, line 57 at r1 (raw file):

		return nil, errors.New("must execute a valid step")
	}
	ns := voc.Namespaces{}

Probably voc.Namespaces{Safe:true}. You don't need to change it concurrently, right?

---

query/linkedql/property_path.go, line 13 at r1 (raw file):

type propertyPathI interface {
	BuildPath(qs graph.QuadStore, ns *voc.Namespaces) (*path.Path, error)

Hmm, maybe we should define a linkedql.QueryContext and pass it here instead? It will contain the voc.Namespaces, query.Collation and maybe other stuff later.

---

query/linkedql/registry.go, line 46 at r3 (raw file):

		panic("only structs are allowed")
	}
	name := Prefix + tp.Name()

This works for linkedql package, but won't work for anything else. With Type() it's possible to define a 3-rd party namespace and 3-rd party Step implementation.

So I propose to either rollback this or allow binding a particular Go package to a RDF namespace. In the second case, Prefix will be taken based on the package where a particular Step is defined.

---

query/linkedql/registry_test.go, line 102 at r1 (raw file):

func (s *TestStep) Type() quad.IRI {
	return "cayley:TestStep"

Yeah, this was here for a reason described in the other thread: it should be possible to define other namespaces.

---

query/linkedql/voc_util.go, line 10 at r7 (raw file):

// AbsoluteIRI uses given ns to resolve short IRIs to their full form
func AbsoluteIRI(iri quad.IRI, ns *voc.Namespaces) quad.IRI {
	return quad.IRI(ns.FullIRI(string(iri)))

iri.FullWith(ns), so you don't need to define this helper, in fact

---

query/linkedql/voc_util.go, line 15 at r7 (raw file):

// AbsoluteValue uses given ns to resolve short IRIs and types in typed strings to their full form
func AbsoluteValue(value quad.Value, ns *voc.Namespaces) quad.Value {
	switch v := value.(type) {

Deserves a helper in quad package.

---

query/linkedql/operators.go, line 17 at r3 (raw file):

}

var _ Operator = (*RegExp)(nil)

Regexp implementation check should probably still be here.

[iddan]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/registry.go, line 46 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

This works for linkedql package, but won't work for anything else. With Type() it's possible to define a 3-rd party namespace and 3-rd party Step implementation.

So I propose to either rollback this or allow binding a particular Go package to a RDF namespace. In the second case, Prefix will be taken based on the package where a particular Step is defined.

I don't see a reason why we need this complication right now. The registry is owned by the linkedql package and so it is fair to couple a namespace with it.

[iddan]

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/registry_test.go, line 102 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Yeah, this was here for a reason described in the other thread: it should be possible to define other namespaces.

I don't agree with this statement. The LinkedQL registry is private.

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

internal/linkedql/schema/schema.go, line 27 at r8 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Probably worth changing to t.Kind() == reflect.Map.

I'd like to be precise about the Go type translation here

---

internal/linkedql/schema/schema.go, line 28 at r8 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Should use voc.Prefix. Also, are you sure it's defined there?

Actually, this type might be incorrect. I got a nice answer in StackOverflow about how to represent a map of literal with RDF:
https://stackoverflow.com/questions/59510977/how-to-represent-a-map-of-literal-keys-in-rdf

---

query/linkedql/voc_util.go, line 10 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

iri.FullWith(ns), so you don't need to define this helper, in fact

Done.

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/iter_tags.go, line 16 at r2 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Why do you export those? To make a separate steps package work.

There is no good reason. Will revert.

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/linkedql.go, line 57 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Probably voc.Namespaces{Safe:true}. You don't need to change it concurrently, right?

Right. Well change

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/property_path.go, line 13 at r1 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Hmm, maybe we should define a linkedql.QueryContext and pass it here instead? It will contain the voc.Namespaces, query.Collation and maybe other stuff later.

It can clean up signatures.

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/voc_util.go, line 15 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Deserves a helper in quad package.

Agree. Can it be done in a different PR though?

[iddan]

Reviewable status: 50 of 55 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)

---

query/linkedql/operators.go, line 17 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Regexp implementation check should probably still be here.

Not sure I understand which code change are you referring here.

[dennwc]

Reviewed 4 of 5 files at r11, 1 of 1 files at r12.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dennwc and @iddan)

---

internal/linkedql/schema/schema.go, line 27 at r8 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I'd like to be precise about the Go type translation here

Might be reasonable. But just to clarify, the code that uses this function will use perfectly fine with any map, right? It will just pass it to json.Marshal or its equivalent.

---

query/linkedql/registry.go, line 46 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I don't see a reason why we need this complication right now. The registry is owned by the linkedql package and so it is fair to couple a namespace with it.

Then we shouldn't export Register at all. Having said that, I see no technical reason to disallow registering custom ones.

---

query/linkedql/voc_util.go, line 15 at r7 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

Agree. Can it be done in a different PR though?

Sure. Can we make it unexported then and add a TODO to move it later.

---

query/linkedql/operators.go, line 17 at r3 (raw file):

var _ Operator = (*RegExp)(nil)

this part was remove, but wasn't added back in a different file

[iddan]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dennwc)

---

internal/linkedql/schema/schema.go, line 27 at r8 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Might be reasonable. But just to clarify, the code that uses this function will use perfectly fine with any map, right? It will just pass it to json.Marshal or its equivalent.

Yes

---

query/linkedql/registry.go, line 46 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Then we shouldn't export Register at all. Having said that, I see no technical reason to disallow registering custom ones.

I think for the sake of simplicity. It's not an API I think we should support.

---

query/linkedql/voc_util.go, line 15 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Sure. Can we make it unexported then and add a TODO to move it later.

Cool

---

query/linkedql/operators.go, line 17 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

var _ Operator = (*RegExp)(nil)

this part was remove, but wasn't added back in a different file

Things got a little mixed up but eventually, it is refactored out so it will be okay

[iddan]

In another PR we are going to make all the names in LinkedQL private as the language should only be consumed

[iddan]

"Context" will not be an actual step in the schema. Rather later we'd refactor it to be implemented per client as it is more of a JSON-LD serialization thing than a logical step.

[dennwc]

As agreed, merging earlier, feedback will be addressed in the following PRs.