GH-5231: fix poor query performance for hasStatements() in FedX #5232
Conversation
The previous implementation of the FedXConnection was delegating
"hasStatements()" to the implementation of "getStatements()", where the
latter was actually fetching data from the federation members.
For checks hasStatements() checks like {null, rdf:type, null} or even
{null, null, null} the implementation is problematic as it would fetch
all data matching the pattern from the federation members, only to
answer if it actually exists.
We now make use of "existence" check on the federation members, and can
actually rely on the source selection cache for this.
Unit test coverage has been added.
| /** | ||
| * Returns the accessible federation members in the context of the query. By default this is all federation members. | ||
| * | ||
| * @param queryInfo | ||
| * @return | ||
| */ | ||
| protected List<Endpoint> getAccessibleFederationMembers(QueryInfo queryInfo) { | ||
| return federationContext.getFederation().getMembers(); | ||
| } |
There was a problem hiding this comment.
Can you explain more why this needs to be protected. I didn't see that it's override anywhere in the code.
There was a problem hiding this comment.
Sure. We make use of specializations of FederationEvalStrategy to validate new optimizations (before contributing them back to RDF4J) or add additional (special) ones applicable to our use-cases. Without having this method accessible in sub-classes, we would need to override the two methods entirely (get/hasStatementsInternal) as we have a specialization for accessible members, currently validating resilience, but looking ahead also for policies/permissions. Does this help?
There was a problem hiding this comment.
Sound good. Can you update the docs to reflect that this is meant to be an extension point for subclasses. If you are unsure if you need to change it in future you can annotate it as experimental if you want.
There was a problem hiding this comment.
Good suggestion, done
|
Btw, as a rule of thumb we usually consider performance issues as bugs and allow them in bug fix releases. |
|
I cancelled the test that was taking too long. It's an issue related to some very specific edge cases when connections and resources are not closed before trying to shutdown the ShaclSail. I have a PR open for trying to fix it, but it's a bit tricky. |
GitHub issue resolved: #5231
The previous implementation of the FedXConnection was delegating "hasStatements()" to the implementation of "getStatements()", where the latter was actually fetching data from the federation members.
For checks hasStatements() checks like {null, rdf:type, null} or even {null, null, null} the implementation is problematic as it would fetch all data matching the pattern from the federation members, only to answer if it actually exists.
We now make use of "existence" check on the federation members, and can actually rely on the source selection cache for this.
Unit test coverage has been added.
PR Author Checklist (see the contributor guidelines for more details):
mvn process-resourcesto format from the command line)