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

Add link conditions for 'stop-at' expression in ExploreRecursive selector #214

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

willscott
Copy link
Member

allows selectors of the form:

{
		"R": {
			"l": {
				"none": 0
			},
			":>": {
				"a": {
					">": {
						"@": {}
					}
				}
			},
			"!": {
				"/": {
                                       "/":"bafyreifepiu23okq5zuyvyhsoiazv2icw2van3s7ko6d3ixl5jx2yj2yhu"
                                }
			}
		}
	}

Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

Mostly questions; one thing I'm pretty sure is a bug.

traversal/selector/condition.go Outdated Show resolved Hide resolved
traversal/selector/condition.go Show resolved Hide resolved
traversal/selector/exploreRecursive.go Outdated Show resolved Hide resolved
traversal/selector/exploreRecursive.go Outdated Show resolved Hide resolved
@willscott willscott requested a review from warpfork August 3, 2021 09:35
* don't skip decides based on stop_at
* don't push explore context into parse context for parsing condition
Copy link
Collaborator

@warpfork warpfork left a comment

Choose a reason for hiding this comment

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

Cool, I think I'm good with this.

More tests would be nice, but Will faught me out-of-band about what a pain that is right now, and we agree to punt until someone has time to renovate all these selector tests to be serial-fixture-driven. (Tests written after that point will be both A) easier to write, and B) more useful as specs and to ensure other implementations are compatible.) So: onward.

@willscott
Copy link
Member Author

Note: this allows stopping recursion at a sub-dag, but doesn't fully de-duplicate 'only new stuff not in the sub-dag i already have' when doing a graph sync which is something you might hope for.

Imagine you're doing something like synchronizing a blockchain, and you want to express 'i already have things up to block '. where each block links to a state tree that evolves over blocks. The functionality of this stop-at isn't quite what you might intuitively what which would be 'don't recurse into parts of the state tree on the new blocks that haven't changed since the block i already have'. The problem with that is that you don't know what all those cids are. You could have a selector to stop at 'all the cids', but that's a huge list to transfer, or you could imagine a form of selector to ask the other side to actually form a set out of the subdag represented by the block you have and not send any of those again, but that's asking for a lot of of computational work from the other side (it would have to walk the full history of the blockchain to generate such a set).
This is an initial compromise, but important to realize that how this selector can be used implies the data structure has been created with some care.

@warpfork
Copy link
Collaborator

warpfork commented Aug 4, 2021

Yeah, I think I could still also image a feature someday which might propose something like:

type TraversalInstructions struct {
    selector Selector
    stopAtAny [Link]
}

Such a feature would differ from this by putting all the stop points in one place towards the root of a protocol conversation, and taking global effect for the walk (e.g. you don't have to know where in the topology the stop is at), and also by being a set (whereas to get that by extending the condition feature, we'd be presumably be implementing 'or' clauses, and I suspect that might become... harder to optimize).

This stopAt feature using Condition is neither the same as, nor in conflict with, such a possible feature. :)

@willscott willscott merged commit e44329e into master Aug 4, 2021
@willscott willscott deleted the selector_condition branch August 4, 2021 10:11
@warpfork
Copy link
Collaborator

I am looking one more time at the specification that we have for Condition in the Selector system. It's... not well specified. I want to remark on this briefly here in case this comes up again in the near future.


it is not well specified

The current degree of specification we have is this excerpt from schema for the Condition union:

	| Condition_HasValue "=" # will need to contain a kinded union, lol.  these conditions are gonna get deep.
	| Condition_HasKind "%" # will ideally want to refer to the DataModel ReprKind enum...!
	| Condition_IsLink "/" # will need this so we can use it in recursions to say "stop at CID QmFoo".

There is no actual specification, at this time, of what any of those types (Condition_IsLink, etc) ... are.

(The same doc also says """The "Condition" system is not fully specified -- it is a placeholder awaiting further design.""" under the "known issues" heading. It's highly disclaimed.)


why I'm wondering about this

I don't really know why Condition_IsLink exists distinctly from those other two.

The name would seem to imply it's a special case of Condition_HasKind?

Or, if it's doing equality check, as the comment seems to suggest, I don't know why Condition_HasValue wouldn't be able to do that.

(And to be clear: I wrote all these lines. I'm saying I don't know what I was thinking at the time. This looks like a sketch kicked out by a tired brain that never ever got reviewed.)


what we seem to have locked in on, with this PR

We seem to have decided that this would be appended to the Selector schema:

type Condition_IsLink link

And that Condition_IsLink is really just an equality check.

We're just quietly ignoring the fact that Condition_HasValue (if we bothered to finish specifying it, which, again, we haven't yet) would probably also suffice for doing an equality check.

Okay. 🤷

This is probably fine. If it is duplicate feature, we're pushing a bit of silly work into future implementors, but... not very much. There's a lot going on right now, and this is not something I think anyone contributing in the area is willing to chase right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants