Feature/281 plugin system to choose storagemanager implementation#301
Conversation
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
fe23b6a to
1742824
Compare
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
2 times, most recently
from
763dc26 to
39cd813
Compare
|
MaillPierre
left a comment
MaillPierre
left a comment
There was a problem hiding this comment.
Please add a section to the main README on:
- How to switch between storage managers (taking graph and Memory as examples)
- What to implement to create your own storage manager
| * | ||
| * @return the plugin priority | ||
| */ | ||
| default int getPriority() { |
There was a problem hiding this comment.
Could you add a comment in the class description about the intended usage of priority ?
| * | ||
| * @return {@code true} if transaction support is enabled | ||
| */ | ||
| public boolean isTransactionSupport() { |
There was a problem hiding this comment.
minor fix: "supportsTransaction" or "hasTransactionSupport"
|
|
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
4b33ed7 to
ed1db20
Compare
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
ed1db20 to
76fecad
Compare
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
76fecad to
6466b3b
Compare
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
6466b3b to
e4d5ca4
Compare
|
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
e4d5ca4 to
55fb41d
Compare
abdessamad-abdoun
force-pushed
the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
from
55fb41d to
047d03b
Compare
| * @return the ValueFactory instance | ||
| */ | ||
| @Override | ||
| public ValueFactory valueFactory() { |
There was a problem hiding this comment.
Remove this redundant method which is the same as a default one.
| import fr.inria.corese.core.kgram.api.core.Edge; | ||
| import fr.inria.corese.core.kgram.api.core.Node; | ||
| import fr.inria.corese.core.next.data.api.*; | ||
| import fr.inria.corese.core.next.storagemanager.api.plugin.PluginException; |
There was a problem hiding this comment.
The import fr.inria.corese.core.next.storagemanager.api.plugin.PluginException is never used
| // Use Graph.addTriple() or Graph.add() instead of create() + addEdge() | ||
| // This bypasses the Edge creation issue | ||
| Edge edge; | ||
| if (context == null) { |
There was a problem hiding this comment.
The named-graph branch seems to call the wrong Graph.addEdge overload. The 4-arg signature is (source, subject, predicate, value), but this passes (subject, predicate, object, context). That means model.add(s, p, o, ctx) will not insert into ctx as intended. Could we swap this to graph.addEdge(context, subject, predicate, object) and add a regression test covering named-graph inserts?
Edge edge;
if (context == null) {
edge = graph.addEdge(subject, predicate, object);
} else {
edge = graph.addEdge(context, subject, predicate, object);
}| @@ -78,14 +95,20 @@ public Set<Statement> find(Resource s, IRI p, Value o, Resource[] contexts) { | |||
| edges = graph.getEdgesRDF4J(subject, predicate, object); | |||
There was a problem hiding this comment.
I think the context semantics should be explicit and consistent across backends:
nullcontext array => wildcard- empty context array => wildcard
{null}=> default graph- mixed arrays such as
{ctx1, ctx2, null}=> union ofctx1,ctx2, and the default graph
At the moment, non-empty context arrays containing null are forwarded as raw null values to the Graph backend, and that path does not handle them correctly. Could we normalize null entries to the Corese default graph node here and add regression tests for these four cases?
| * @throws PluginException if the memory storage fails to initialize | ||
| */ | ||
| public Model createModel(String storageType) throws PluginException { | ||
| StorageConfig config = switch (storageType) { |
There was a problem hiding this comment.
I think this is still more hardcoded than the plugin system suggests.
StoragePluginManager is designed to discover storage backends dynamically, so in principle adding a new StoragePlugin should be enough to make a new backend available. But ModelFactory.createModel(String) still only knows "memory" and "graph" through this switch.
So if someone adds a new plugin later, StoragePluginManager could support it, but ModelFactory would still reject it before the plugin manager is even called.
There is also a smaller inconsistency here: the plugins use case-insensitive matching (equalsIgnoreCase), but this switch is case-sensitive.
Would it make sense to keep this method as a convenience for the built-in backends and also add a createModel(StorageConfig config) overload that delegates directly to StoragePluginManager?
|
|
MaillPierre
added
the
Refactoring
| @@ -287,10 +361,6 @@ private Edge statementToEdge(Statement stmt) { | |||
|
|
|||
There was a problem hiding this comment.
I think the same 4-argument ordering issue still remains here in the delete/contains path.
Graph.create(...) expects (source, subject, predicate, value), but this code still passes (subject, predicate, object, context). As a result, statements with a context are not converted to an Edge with the expected graph/source node layout.
Since both remove() and contains() rely on statementToEdge(), could we align this with the fixed add() path and build the edge as (context, subject, predicate, object) when a context is present?
Could we also add tests for these cases (especially with a context) to ensure remove() and contains() use the correct edge construction?
|
| Value object = nodeToValue(objectNode); | ||
|
|
||
| // Context (named graph) | ||
| Node graphNode = edge.getGraph(); |
There was a problem hiding this comment.
I think the default-graph mapping is still not fully symmetric here.
The query-side behavior now looks like:
null/ empty context array => wildcard{null}=> default graph- mixed arrays such as
{ctx1, ctx2, null}=> union including the default graph
If that is the intended contract, then the statement/edge conversion should follow the matching mapping as well:
Statementwithnullcontext -> Corese default graph node- Corese default graph node ->
nullstatement context - named graph node -> corresponding
Resource
At the moment, statementToEdge() still maps a null statement context to graph.create(null, ...), and edgeToStatement() converts any non-null graph node directly to a Resource. Could we special-case the default graph node in both directions?
private Statement edgeToStatement(Edge edge) {
if (edge == null) {
throw new IllegalArgumentException("Edge cannot be null");
}
Node subjectNode = edge.getSubjectNode();
Node predicateNode = edge.getEdgeNode();
Node objectNode = edge.getObjectNode();
if (subjectNode == null) {
throw new IllegalArgumentException("Edge subject node is null");
}
if (predicateNode == null) {
throw new IllegalArgumentException("Edge predicate node is null");
}
if (objectNode == null) {
throw new IllegalArgumentException("Edge object node is null");
}
Resource subject = nodeToResource(subjectNode);
IRI predicate = nodeToIRI(predicateNode);
Value object = nodeToValue(objectNode);
Node graphNode = edge.getGraph();
Resource context = (graphNode == null || graph.isDefaultGraphNode(graphNode))
? null
: nodeToResource(graphNode);
if (context == null) {
return valueFactory.createStatement(subject, predicate, object);
} else {
return valueFactory.createStatement(subject, predicate, object, context);
}
}| Node object = valueToNode(stmt.getObject()); | ||
|
|
||
| Resource ctxResource = stmt.getContext(); | ||
| Node context = (ctxResource != null) ? resourceToNode(ctxResource) : null; | ||
|
|
||
| return graph.create(subject, predicate, object, context); | ||
| if (ctxResource == null) { | ||
| return graph.create(null, subject, predicate, object); | ||
| } else { | ||
| Node context = resourceToNode(ctxResource); | ||
| return graph.create(context, subject, predicate, object); | ||
| } |
There was a problem hiding this comment.
private Edge statementToEdge(Statement stmt) {
Node subject = resourceToNode(stmt.getSubject());
Node predicate = iriToNode(stmt.getPredicate());
Node object = valueToNode(stmt.getObject());
Resource ctxResource = stmt.getContext();
Node context = (ctxResource != null)
? resourceToNode(ctxResource)
: graph.getDefaultGraphNode();
return graph.create(context, subject, predicate, object);
}
remiceres
left a comment
remiceres
left a comment
There was a problem hiding this comment.
Could we add a regression test covering the default-graph round-trip here?
A useful case would be:
- insert a statement with no context /
{null} - query it back
- verify that the returned
StatementhasgetContext() == null, not an explicit default-graph resource
That would help lock in the intended Statement <-> Graph context conversion semantics.
|
MaillPierre
deleted the
feature/281-plugin-system-to-choose-storagemanager-implementation
branch
3 participants
No description provided.