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
JENA-1560: PrefixMappingUtils #432
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit-picking, but only typos in docs. Feel free to leave as-is if not important 👍
* Analyse the graph to see which prefixes of the graph are in use. | ||
* <p> | ||
* In the case of overlapping prefixes (where one prefix declaration is has an initial | ||
* URI string which imatches another prefix declaration), all are included, though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/imatches/it matches
?
* use. | ||
* <p> | ||
* In the case of overlapping prefixes (where one prefix declaration is has an initial | ||
* URI string which imatches another prefix declaration), all are included, though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/imatches/it matches
?
|
||
// Map URI to prefix, with partial lookup (all uri keys that partly match the URI) | ||
Trie<String> trie = new Trie<>() ; | ||
// Chnage this to "add(uri, uri)" to calculate the uris. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Chnage/Change
String uri = node.getURI() ; | ||
// Get all prefixes whose URIs are candidates | ||
List<String> hits = trie.partialSearch(uri) ; | ||
if ( hits == null || hits.isEmpty() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think partialSearch
never returns null
, but harmless to leave the check here.
Thanks - all review points done. I amended to PR to keep it to one commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor stuff, nothing to spend too much time with.
* <p> | ||
* The prefix mappings of the two graphs are not connected. | ||
* Later changes to the prefix mapping of the original graph are not reflected in the returned graph. | ||
* Modifications to the triples conatained in the underlying graph are reflected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo "conatained" => "contained"
public class PrefixMappingUtils { | ||
/** | ||
* Return a read-only graph that has the same data (RDF triples) as the one given, but has a | ||
* prefix mapping that only includes "in use " prefixes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to put a link here to calcInUsePrefixMapping
below just in case someone wants to the precise meaning of "in use".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* | ||
* @see #calcInUsePrefixMapping(Graph, PrefixMapping) | ||
*/ | ||
public static PrefixMapping calcInUsePrefixMappingTTL(Graph graph, PrefixMapping prefixMapping) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a TODO to factor out common logic between this and calcInUsePrefixMapping
?
|
||
// Get all under the pref | ||
List<String> hits = trie.partialSearch(pref) ; | ||
if ( hits == null || hits.isEmpty() ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at Trie::partialSearch
, I don't think hits
can be null
here. I guess why you don't check for null
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just being defensive.
// inUse.add(ns) ; | ||
} | ||
|
||
private static void print(Set<String> inUsePrefixURIs, PrefixMapping prefixMapping) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we leave this in? It looks like debugging code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm took a while to get right and being about to print the state was useful. I'll add a comment that's it for development assistance. Maybe it will never needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The algorithm took a while to get right and being about to print the state was useful. I'll add a comment that's it for development assistance. Maybe it will never needed.
Some utility code that can be useful.