Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LinkedQL #878

Merged
merged 148 commits into from Dec 15, 2019
Merged

LinkedQL #878

merged 148 commits into from Dec 15, 2019

Conversation

@iddan
Copy link
Collaborator

iddan commented Oct 17, 2019

Resolves #877
Resolves #835
Resolves #852
Resolves #311

This change is Reviewable

@iddan iddan added this to the v0.8.1 milestone Oct 17, 2019
@iddan iddan marked this pull request as ready for review Oct 19, 2019
@iddan iddan requested a review from dennwc as a code owner Oct 19, 2019
@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Oct 19, 2019

Status of implementation:

  • graph.Morphism()
    • Implementation
    • Test
  • graph.Vertex([nodeId],[nodeId]...)
    • Implementation
    • Test
  • path.and(path)
    • Implementation
    • Test
  • path.as(tags)
    • Implementation
    • Test
  • path.back([tag])
    • Implementation
    • Test
  • path.both([predicatePath], [tags])
    • Implementation
    • Test
  • path.count()
    • Implementation
    • Test
  • path.difference(path)
    • Implementation
    • Test
  • path.except(path)
    • Implementation
    • Test
  • path.filter(args)
    • Implementation
    • Test
  • path.follow(path)
    • Implementation
    • Test
  • path.followR(path)
    • Implementation
    • Test
  • path.followRecursive(*)
    • Implementation
    • Test
  • path.has(predicate, object)
    • Implementation
    • Test
  • path.hasR(*)
    • Implementation
    • Test
  • path.in([predicatePath], [tags])
    • Implementation
    • Test
  • path.inPredicates()
    • Implementation
    • Test
  • path.intersect(path)
    • Implementation
    • Test
  • path.is(node, [node..])
    • Implementation
    • Test
  • path.labelContext([labelPath], [tags])
    • Implementation
    • Test
  • path.labels()
    • Implementation
    • Test
  • path.limit(limit)
    • Implementation
    • Test
  • path.out([predicatePath], [tags])
    • Implementation
    • Test
  • path.outPredicates()
    • Implementation
    • Test
  • path.save(predicate, tag)
    • Implementation
    • Test
  • path.saveInPredicates(tag)
    • Implementation
    • Test
  • path.saveOpt(*)
    • Implementation
    • Test
  • path.saveOptR(*)
    • Implementation
    • Test
  • path.saveOutPredicates(tag)
    • Implementation
    • Test
  • path.saveR(*)
    • Implementation
    • Test
  • path.skip(offset)
    • Implementation
    • Test
  • path.tag(tags)
    • Implementation
    • Test
  • path.tagArray(*)
    • Implementation
    • Test
  • path.value()
    • Implementation
    • Test
  • path.tagValue()
    • Implementation
    • Test
  • path.union(path)
    • Implementation
    • Test
  • path.unique()
    • Implementation
    • Test
  • path.order()
    • Implementation
    • Test
@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Oct 19, 2019

@dennwc I'd like your help with Morphism. I'll try to reduce panics and cover more parts of the code with testing soon but I think we can start the review process.

Copy link
Member

dennwc left a comment

Reviewed 8 of 9 files at r3.
Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @iddan)


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

		_, ok = a["@value"].(string)
		if ok {
			panic("Doesn't support special literals yet")

Will need to remove panics before merging.


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

		}
	}
	return nil, errors.New("Couldn't parse rawValue to a quad.Value")

fmt.Errorf("cannot parse JSON-LD value: %#v", a)


query/linkedql/session.go, line 31 at r3 (raw file):

		return nil, err
	}
	iterator := step.BuildIterator(s.qs)

Just return it directly.


query/linkedql/session.go, line 36 at r3 (raw file):

// ShapeOf returns for given query a Shape
func (s *Session) ShapeOf(query string) (interface{}, error) {

We need to rebase (not merge!) this branch and remove the ShapeOf - I deprecated it.


query/linkedql/steps.go, line 15 at r3 (raw file):

)

const namespace = "http://cayley.io/linkedql#"
const (
  namespace = ...
  prefix = ...
)

query/linkedql/steps.go, line 60 at r3 (raw file):

}

// Vertex corresponds to g.Vertex() and g.V()

Comment should be full sentences (end with a dot).


query/linkedql/steps.go, line 87 at r3 (raw file):

// BuildIterator implements Step
func (s *Morphism) BuildIterator(qs graph.QuadStore) query.Iterator {
	panic("Not implemented")

Re: helping with Morhism, please remove it from the PR. We will add it separately. Same for Recursive.


query/linkedql/steps.go, line 106 at r3 (raw file):

	fromIt, ok := s.From.BuildIterator(qs).(*ValueIterator)
	if !ok {
		panic("Out must be called from ValueIterator")

We should change the interface to return an error from BuildIterator.


query/linkedql/steps.go, line 138 at r3 (raw file):

// TagArray corresponds to .tagArray()
type TagArray struct {

Maybe let's move those "finals" to a separate file?


query/linkedql/steps.go, line 177 at r3 (raw file):

// Value corresponds to .value()
// TODO(iddan): Limit one result

You can wrap a Limit iterator when building it.


query/linkedql/steps.go, line 193 at r3 (raw file):

		panic("Value must be called from ValueIterator")
	}
	// TODO(@iddan): support non iterators for query result

TODO(iddan) (no need for @)


query/linkedql/steps.go, line 200 at r3 (raw file):

type Intersect struct {
	From        Step `json:"from"`
	Intersectee Step `json:"intersectee"`

That's a weird name. Maybe call Sub and allow a slice here?


query/linkedql/steps.go, line 341 at r3 (raw file):

// BuildIterator implements Step
func (s *LessThan) BuildIterator(qs graph.QuadStore) query.Iterator {
	panic("Can't BuildIterator for " + s.Type())

We should either remove it from the current PR, or implement it properly.


query/linkedql/steps.go, line 391 at r3 (raw file):

// RegExp corresponds to regex()
type RegExp struct {
	Expression  string `json:"expression"`

json:"pattern"


query/linkedql/steps.go, line 439 at r3 (raw file):

	switch filter := s.Filter.(type) {
	case *LessThan:
		return NewValueIterator(fromIt.path.Filter(iterator.Operator(iterator.CompareLT), filter.Value), qs)

You don't need a cast to iterator.Operator here I believe.


query/linkedql/steps.go, line 487 at r3 (raw file):

// FollowReverse corresponds to .followR()
type FollowReverse struct {

Maybe add a Reverse flag to Follow instead?


query/linkedql/steps.go, line 530 at r3 (raw file):

	From   Step         `json:"from"`
	Via    Step         `json:"via"`
	Values []quad.Value `json:"values"`

Change it to Step as well? We'll need to support has(..., path) eventually.


query/linkedql/steps.go, line 621 at r3 (raw file):

// LabelContext corresponds to .labelContext()
type LabelContext struct {

We should remove it from this PR, LabelContext is a bit tricky.


query/linkedql/steps.go, line 711 at r3 (raw file):

// BuildIterator implements Step
// TODO(iddan): Default tag to Via

Just don't pass s.Tag to .Save() if it's empty.


query/linkedql/steps.go, line 745 at r3 (raw file):

// SaveOptional corresponds to .saveOpt()
type SaveOptional struct {

optional flag on Save?


query/linkedql/steps.go, line 815 at r3 (raw file):

// SaveReverse corresponds to .saveR()
type SaveReverse struct {

reverse flag on Save?


query/linkedql/steps.go, line 840 at r3 (raw file):

// Skip corresponds to .skip()
type Skip struct {

I'd prefer to name it Page and join it with Limit (allow both offset and limit fields).


query/linkedql/steps.go, line 862 at r3 (raw file):

type Union struct {
	From      Step `json:"from"`
	Unionized Step `json:"unionized"`

The name is a bit weird. Maybe call Sub and allow a slice of steps here?


query/linkedql/steps_test.go, line 20 at r3 (raw file):

	{
		name: "All Vertices",
		data: []quad.Quad{

This specific "dataset" repeats a lot. Define a global with this slice and use it here.


query/linkedql/tag_array_iterator.go, line 11 at r3 (raw file):

type TagArrayIterator struct {
	valueIterator *ValueIterator

sub, or it, or valueIt - no reason to have those large names


query/linkedql/tag_array_iterator.go, line 25 at r3 (raw file):

		tags[tag] = it.valueIterator.namer.NameOf(ref)
	}
	// TODO(iddan): collect tags of current iteration

What do you mean here?


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

func NewValueIterator(p *path.Path, namer graph.Namer) *ValueIterator {
	return &ValueIterator{namer, p, nil}

Name field here and remove nil.

@iddan iddan changed the base branch from master to inference Oct 22, 2019
@iddan iddan changed the base branch from inference to master Oct 22, 2019
Copy link
Collaborator Author

iddan left a comment

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


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

Previously, dennwc (Denys Smirnov) wrote…

Will need to remove panics before merging.

Done


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

Previously, dennwc (Denys Smirnov) wrote…

fmt.Errorf("cannot parse JSON-LD value: %#v", a)

Done


query/linkedql/session.go, line 31 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Just return it directly.

Done.


query/linkedql/session.go, line 36 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

We need to rebase (not merge!) this branch and remove the ShapeOf - I deprecated it.

Done.


query/linkedql/steps.go, line 15 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…
const (
  namespace = ...
  prefix = ...
)

Done.


query/linkedql/steps.go, line 87 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Re: helping with Morhism, please remove it from the PR. We will add it separately. Same for Recursive.

Done.


query/linkedql/steps.go, line 106 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

We should change the interface to return an error from BuildIterator.

Done.


query/linkedql/steps.go, line 138 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Maybe let's move those "finals" to a separate file?

Done.


query/linkedql/steps.go, line 177 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

You can wrap a Limit iterator when building it.

Done.


query/linkedql/steps.go, line 193 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

TODO(iddan) (no need for @)

Done.


query/linkedql/steps.go, line 341 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

We should either remove it from the current PR, or implement it properly.

Done.


query/linkedql/steps.go, line 391 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

json:"pattern"

Done.


query/linkedql/steps.go, line 439 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

You don't need a cast to iterator.Operator here I believe.

Done.


query/linkedql/steps.go, line 487 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Maybe add a Reverse flag to Follow instead?

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.


query/linkedql/steps.go, line 530 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Change it to Step as well? We'll need to support has(..., path) eventually.

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.


query/linkedql/steps.go, line 621 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

We should remove it from this PR, LabelContext is a bit tricky.

Done.


query/linkedql/steps.go, line 711 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Just don't pass s.Tag to .Save() if it's empty.

I was not able to do it as it strictly expects a string tag.


query/linkedql/steps.go, line 815 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

reverse flag on Save?

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.


query/linkedql/steps.go, line 840 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I'd prefer to name it Page and join it with Limit (allow both offset and limit fields).

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.


query/linkedql/steps_test.go, line 20 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

This specific "dataset" repeats a lot. Define a global with this slice and use it here.

Done.

Copy link
Member

dennwc left a comment

Reviewed 9 of 12 files at r6.
Reviewable status: 9 of 11 files reviewed, 14 unresolved discussions (waiting on @dennwc and @iddan)


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

Previously, iddan (Iddan Aaronsohn) wrote…

Done

I still see the panic in the code.


query/linkedql/session.go, line 36 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

Done.

Still see some merges in the log. Will rebase manually a bit later.


query/linkedql/steps.go, line 487 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.

Note that we must solve it before releasing v0.8. As soon as we release it we will have to support this API for a relatively long time.

Re: matching Gizmo 1:1, I don't really understand why it should be the case. This is a protocol for queries, not a query plan for users to see. So if you consider an effort to implement it in another client, say Java, developers will benefit from having less types in the spec, not more. WDYT?


query/linkedql/steps.go, line 711 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I was not able to do it as it strictly expects a string tag.

Sorry, confused it with Gizmo's implementation.


query/linkedql/steps.go, line 347 at r6 (raw file):

		return nil, err
	}
	switch filter := s.Filter.(type) {

This should really be an implementation of some interface by LessThan and others.

@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Oct 25, 2019

I see what you say. But I think the steps should reflect the API exposed. Specifically, I think .skip().limit() is a good API. I think two guidelines are similarity to Gremlin API and simplicity.

@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Oct 25, 2019

@dennwc I think I got rid of the last problematic panic()

Copy link
Collaborator Author

iddan left a comment

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


query/linkedql/steps.go, line 200 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

That's a weird name. Maybe call Sub and allow a slice here?

Will do the slice, I think the name Sub is not indicative enough. How about intersected?


query/linkedql/steps.go, line 487 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Note that we must solve it before releasing v0.8. As soon as we release it we will have to support this API for a relatively long time.

Re: matching Gizmo 1:1, I don't really understand why it should be the case. This is a protocol for queries, not a query plan for users to see. So if you consider an effort to implement it in another client, say Java, developers will benefit from having less types in the spec, not more. WDYT?

I agree we should aim for a better API but as long as a method is exposed it should have a matching step this will reduce the overhead of understanding the tree and the development of libraries in most languages.


query/linkedql/steps.go, line 347 at r6 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

This should really be an implementation of some interface by LessThan and others.

Done.

Copy link
Collaborator Author

iddan left a comment

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


query/linkedql/steps.go, line 60 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

Comment should be full sentences (end with a dot).

Done.

Copy link
Collaborator Author

iddan left a comment

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


query/linkedql/steps.go, line 745 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

optional flag on Save?

Done.


query/linkedql/steps.go, line 815 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.

Though about it a little more: I think it's better we will be explicit about the fact that in and out properties are very different. What do you think?


query/linkedql/steps.go, line 840 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

I'd like to preserve the current steps and not create differentiation between Gizmo and LinkedQL. We can iterate on it later.

Though about it a little more: I think offset and limit are good API but we can provide an additional .page() step that works differently.


query/linkedql/steps.go, line 862 at r3 (raw file):

Unionized

Will change to slice, what's wrong with unionized? I think Sub is a nonindicative name.


query/linkedql/tag_array_iterator.go, line 11 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

sub, or it, or valueIt - no reason to have those large names

Done.


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

Previously, dennwc (Denys Smirnov) wrote…

Name field here and remove nil.

Done.

Copy link
Member

dennwc left a comment

There is a lot of noise in diffs, so need to review the whole thing again.

Reviewed 3 of 10 files at r8.
Reviewable status: 4 of 11 files reviewed, 11 unresolved discussions (waiting on @dennwc and @iddan)


query/linkedql/registry.go, line 148 at r7 (raw file):

		return quad.String(a), nil
	case int64:
		return quad.TypedString{Value: quad.String(a), Type: quad.IRI(xsdInt)}, nil

quad.Int


query/linkedql/registry.go, line 150 at r7 (raw file):

		return quad.TypedString{Value: quad.String(a), Type: quad.IRI(xsdInt)}, nil
	case float64:
		return quad.TypedString{Value: quad.String(fmt.Sprintf("%f", a)), Type: quad.IRI(xsdFloat)}, nil

quad.Float


query/linkedql/registry.go, line 152 at r7 (raw file):

		return quad.TypedString{Value: quad.String(fmt.Sprintf("%f", a)), Type: quad.IRI(xsdFloat)}, nil
	case bool:
		return quad.TypedString{Value: quad.String(fmt.Sprintf("%t", a)), Type: quad.IRI(xsdBool)}, nil

quad.Bool


query/linkedql/steps.go, line 815 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

Though about it a little more: I think it's better we will be explicit about the fact that in and out properties are very different. What do you think?

I agree in terms of user-facing API (Gizmo), but not from the protocol standpoint.

Because of this tiny bit we will need make two types for all all those steps: Out (Traverse may be a better name), Save, Has, Follow, FollowRecursize. Later we will need to add outE, so we'll need to multiple the number of types again.

So I'd still prefer to add a flag instead. It makes implementation of clients easier in terms of writing actual HTTP client. Clients won't see the protocol anyway. And it's the same amount of work, since we will introduce Out and In in both cases. It doesn't matter if we send it as a flag or not.


query/linkedql/steps.go, line 840 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…

Though about it a little more: I think offset and limit are good API but we can provide an additional .page() step that works differently.

We initially had two separate shapes internally, but in the new API there is only one. It's better for the optimizer to know both parameters in the same operation.

And again, as in discussion about Reverse, users won't notice if it will be one shape or not.


query/linkedql/steps.go, line 862 at r3 (raw file):

Previously, iddan (Iddan Aaronsohn) wrote…
Unionized

Will change to slice, what's wrong with unionized? I think Sub is a nonindicative name.

I don't think this word even exists :)

@dennwc dennwc force-pushed the feature/linkedql branch from 2ae3331 to fd0eabe Nov 3, 2019
Copy link
Collaborator Author

iddan left a comment

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


query/linkedql/registry.go, line 148 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

quad.Int

quad.Int definition is not JSON-LD compatible. We should either change or leave this.


query/linkedql/registry.go, line 150 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

quad.Float

quad.Float definition is not JSON-LD compatible. We should either change or leave this.


query/linkedql/registry.go, line 152 at r7 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

quad.Bool

quad.Bool definition is not JSON-LD compatible. We should either change or leave this.


query/linkedql/steps.go, line 815 at r3 (raw file):

Previously, dennwc (Denys Smirnov) wrote…

I agree in terms of user-facing API (Gizmo), but not from the protocol standpoint.

Because of this tiny bit we will need make two types for all all those steps: Out (Traverse may be a better name), Save, Has, Follow, FollowRecursize. Later we will need to add outE, so we'll need to multiple the number of types again.

So I'd still prefer to add a flag instead. It makes implementation of clients easier in terms of writing actual HTTP client. Clients won't see the protocol anyway. And it's the same amount of work, since we will introduce Out and In in both cases. It doesn't matter if we send it as a flag or not.

The client's code is auto-generated based on the schema. The steps names are the names of the methods exposed to the clients.

@dennwc dennwc force-pushed the feature/linkedql branch from 54c9b5b to b47cc6c Nov 9, 2019
@iddan iddan force-pushed the feature/linkedql branch from 2357c3c to 388a058 Nov 14, 2019
@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Nov 20, 2019

@dennwc I think for now we can leave Properties to accept only IRIs until we make changes for morphism.

@iddan

This comment has been minimized.

Copy link
Collaborator Author

iddan commented Nov 30, 2019

@dennwc we should make properties with empty iri set to save all properties

@dennwc dennwc force-pushed the feature/linkedql branch from 59cc665 to d583b4a Dec 15, 2019
@dennwc
dennwc approved these changes Dec 15, 2019
Copy link
Member

dennwc left a comment

Reviewed 2 of 14 files at r12, 1 of 6 files at r13.
Reviewable status: 3 of 14 files reviewed, 3 unresolved discussions (waiting on @dennwc and @iddan)

@dennwc dennwc merged commit 4d89b8a into master Dec 15, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 11 files, 3 discussions left (dennwc, iddan)
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
@dennwc dennwc deleted the feature/linkedql branch Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.