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

Added two new tests with duplicate triples #192

Merged
merged 3 commits into from
Feb 12, 2024
Merged

Conversation

iherman
Copy link
Member

@iherman iherman commented Feb 8, 2024

This PR is to fix #191.

Adds two tests: each contain two identical triples in the input (without and with an identical blank node, respectively); the parsing of the the nq files should make it sure that a dataset is canonicalized, ie, the duplication of triples disappear.

@iherman
Copy link
Member Author

iherman commented Feb 8, 2024

@gkellogg I do not know the details of the github action that reports an error. I would leave that in your able hands...

@gkellogg
Copy link
Member

gkellogg commented Feb 9, 2024

Without objection, I think we should merge this and allow updated implementation reports to be submitted.

@davidlehn
Copy link
Contributor

rdf-canonize passes the tests here and had a local test for this already. It used a variation of the tests here with subject and object blank nodes. Should that be added here too or is it redundant?

Input:

_:s <https://www.example.org/p> _:o .
_:s <https://www.example.org/p> _:o .

Output:

_:c14n1 <ex:p> _:c14n0 .

@gkellogg
Copy link
Member

If those tests would add something not otherwise caught, then we should add them to this PR.

@iherman
Copy link
Member Author

iherman commented Feb 10, 2024

If those tests would add something not otherwise caught, then we should add them to this PR.

True. But I am not sure that tests really add something. The issue to be tested is whether the implementation correctly filters duplicate quads in an nquads representation of a graph, whether those quads/triples include (identical) blank nodes or not. The fact that there is one bnode or there are two does not change the situation imho.

Copy link
Contributor

@yamdan yamdan left a comment

Choose a reason for hiding this comment

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

My implementation also passes the added two tests. I will update the report after the PR is merged.

@pchampin
Copy link
Contributor

FTR, my implementation also passes these tests

@gkellogg gkellogg merged commit 03e68bb into main Feb 12, 2024
1 check passed
@gkellogg gkellogg deleted the add-duplicated-quads-tests branch February 12, 2024 16:09
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.

What is the way to go with duplicate triples?
7 participants