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

Incorrect/inconsistent cardinality calculation #1873

Closed
pulquero opened this issue Jan 23, 2020 · 16 comments · Fixed by #2029
Closed

Incorrect/inconsistent cardinality calculation #1873

pulquero opened this issue Jan 23, 2020 · 16 comments · Fixed by #2029
Assignees
Labels
🐞 bug issue is a bug good first issue issue is a good choice for getting started as an rdf4j contributor ⏩ performance
Milestone

Comments

@pulquero
Copy link

EvaluationStatistics.getCardinality() should use Set vars = pattern.getVars(new HashSet<>()) else
?s ?p ?o and ?s ?s ?s are considered to have the same cardinality.

@barthanssens barthanssens added the 🐞 bug issue is a bug label Jan 23, 2020
@hmottestad
Copy link
Contributor

hmottestad commented Jan 23, 2020

@pulquero can you provide a link to the exact line in the code in github?

@pulquero
Copy link
Author

pulquero commented Jan 23, 2020

@hmottestad
Copy link
Contributor

Thanks

@hmottestad hmottestad added good first issue issue is a good choice for getting started as an rdf4j contributor ⏩ performance labels Jan 28, 2020
@reeshabhranjan
Copy link
Contributor

Can I work on this issue?

@barthanssens
Copy link
Contributor

barthanssens commented Mar 9, 2020

Sure, it's an open source project, and any help is welcome and much appreciated.

If this is your first contribution to RDF4J, there is one small but necessary thing you have to do: register for a (free) Eclipse account and sign a form

See "Legal stuff" in https://github.com/eclipse/rdf4j/blob/master/.github/CONTRIBUTING.md

@hmottestad
Copy link
Contributor

@aschwarte10 is this class org.eclipse.rdf4j.sail.federation.optimizers.EvaluationStatistics used by FedX?

@reeshabhranjan
Copy link
Contributor

So as per I understood the code, and the solution suggested in this issue, I just need to make sure that only distinct elements are in the vars list in this line, right?

@hmottestad
Copy link
Contributor

So as per I understood the code, and the solution suggested in this issue, I just need to make sure that only distinct elements are in the vars list in this line, right?

I believe that is what is required. A test would also be nice.

@reeshabhranjan
Copy link
Contributor

Alright, I will link a pull-request for this shortly.

@reeshabhranjan
Copy link
Contributor

Sure, it's an open source project, and any help is welcome and much appreciated.

If this is your first contribution to RDF4J, there is one small but necessary thing you have to do: register for a (free) Eclipse account and sign a form

See "Legal stuff" in https://github.com/eclipse/rdf4j/blob/master/.github/CONTRIBUTING.md

Thank you @barthanssens for linking the legal procedures.

@reeshabhranjan
Copy link
Contributor

reeshabhranjan commented Mar 17, 2020

I have made some changes to the code (here).
I think since Var has both hashCode and equals defined, the HashSet will handle the duplicate elements.
If this is fine, I would like to proceed with the tests. Also, this will be my first contribution for this repository, so I am a bit unsure about where to write the tests. Should I make a new file under

<project-root>/testsuites/sail/src/main/java/org/eclipse/rdf4j/sail ?

@hmottestad
Copy link
Contributor

How about in a new package optimizers in core/sail/federation/src/test/java/org/eclipse/rdf4j/sail/federation?

@reeshabhranjan
Copy link
Contributor

Alright, I will create a package optimizers and will create a test-file in it.

@reeshabhranjan
Copy link
Contributor

I missed adding the issue number in my first commit, so is that fine or should I do something about it?

@hmottestad
Copy link
Contributor

You can always edit your commit messages afterwards and also squash multiple commits together. Just don't merge anything into your branch.

More importantly is that you sign off on your commits.

Every commit you make in your patch or pull request MUST be "signed off". You do this by adding the -s flag when you make the commit(s).

I found this helpful guide if you've never used sign off before: https://docs.pi-hole.net/guides/github/how-to-signoff/

reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 21, 2020
…inct elements

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 21, 2020
Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 21, 2020
Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 21, 2020
…inct elements, added related tests

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 21, 2020
Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
@reeshabhranjan
Copy link
Contributor

I have linked the pull-request, kindly review it and let me know of any suggestions.

@abrokenjester abrokenjester added this to the 3.1.3 milestone Mar 22, 2020
reeshabhranjan added a commit to reeshabhranjan/rdf4j that referenced this issue Mar 22, 2020
…countConstantVars()` method, as suggested by @jeenbroekstra; added the actual year in the header comment in CardinalityTest.java

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
abrokenjester added a commit that referenced this issue Mar 22, 2020
…lculation

GH-1873 fixing incorrect cardinality calculation
Mirr99 pushed a commit to Mirr99/rdf4j that referenced this issue Mar 24, 2020
…inct elements, added related tests

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
Mirr99 pushed a commit to Mirr99/rdf4j that referenced this issue Mar 24, 2020
Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
Mirr99 pushed a commit to Mirr99/rdf4j that referenced this issue Mar 24, 2020
…countConstantVars()` method, as suggested by @jeenbroekstra; added the actual year in the header comment in CardinalityTest.java

Signed-off-by: Reeshabh Kumar Ranjan <reeshabhkumarranjan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug issue is a bug good first issue issue is a good choice for getting started as an rdf4j contributor ⏩ performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants