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

Change to Triples.goTo(pos) leads to wrong results #84

Closed
RubenVerborgh opened this issue Aug 24, 2017 · 3 comments
Closed

Change to Triples.goTo(pos) leads to wrong results #84

RubenVerborgh opened this issue Aug 24, 2017 · 3 comments
Assignees
Labels
Milestone

Comments

@RubenVerborgh
Copy link
Member

It seems that de30539 breaks functionality. I discovered this when trying to update HDT-Node to v1.2.0, which includes de30539.

The unit test which searches for the pattern ex:s2 null null also returns triples with ex:s1 as a subject: https://travis-ci.org/RubenVerborgh/HDT-Node/jobs/268181699#L611

@MarioAriasGa What was the rationale behind the change?
Are we sure it needed to be absolute? What does that fix?
Should it be reverted or should code in other places be adjusted?

@MarioAriasGa
Copy link
Member

The parameter goTo() was originally meant to be absolute and is used from the HDT-it GUI when scrolling the list of triples. I now see that the ObjectIterator is wrong as well and I didn't change it in that commit.

I suggest leaving goTo() absolute, and adding a skip(amount) method to the iterator and using that instead from HDT-node.

@RubenVerborgh
Copy link
Member Author

Thanks!

I now see that the ObjectIterator is wrong as well and I didn't change it in that commit.

I see, could you start a fix/goto branch or similar and create a PR, so we can track for there?

Can you check whether anything else in the source code depends on goTo?

I suggest leaving goTo() absolute, and adding a skip(amount) method to the iterator and using that instead from HDT-node.

For me, it's the same what HDT-Node uses. The issue was that functionality unrelated to goTo also broke.

Can you let me know then what I should be calling to set an offset?

@RubenVerborgh
Copy link
Member Author

Resolved by #89.

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

No branches or pull requests

2 participants