[
https://issues.apache.org/jira/browse/TINKERPOP-848?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17428419#comment-17428419
]
ASF GitHub Bot commented on TINKERPOP-848:
------------------------------------------
spmallette commented on a change in pull request #1485:
URL: https://github.com/apache/tinkerpop/pull/1485#discussion_r727955226
##########
File path:
gremlin-core/src/main/java/org/apache/tinkerpop/gremlin/structure/io/graphml/GraphMLReader.java
##########
@@ -138,9 +143,11 @@ public void readGraph(final InputStream graphInputStream,
final Graph graphToWri
case GraphMLTokens.DATA:
final String key = reader.getAttributeValue(null,
GraphMLTokens.KEY);
final String dataAttributeName = keyIdMap.get(key);
+ final String defaultValue = defaultValues.get(key);
if (dataAttributeName != null) {
- final String value = reader.getElementText();
+ String elementValue = reader.getElementText();
+ final String value = elementValue.length() ==
0 && defaultValue.length() != 0 ? defaultValue : elementValue;
Review comment:
> This does not look right to me, key should be d_n.
sorry - i guess i'm used to making id/attr.name/key the same. didn't mean to
be confusing in my comment. My point was more that `<data key="d_n"/>` should
not trigger use of the default. It should simply be empty string.
The case for default usage is when the there is no `<data>` element with
`key="d_n"` at all. In that case it should inject the property
"modification=add". You mention that this situation currently leads to:
> there is no default value therefore related events won't be emitted at
all....My implementation will generate NPE in such cases
I think the goal here is to determine if the property key is present or not
in the GraphML for the particular node/edge and then adding it if its not. In
that sense, I think that the bulk of your changes for this should neatly fit
into `XMLEvent.END_ELEMENT` where all the property keys have already been
collected and you just before the vertex is created. Does that all make sense?
##########
File path: gremlin-core/src/test/resources/graphml/sample.graphml.xml
##########
@@ -0,0 +1,367 @@
+<?xml version="1.0" encoding="UTF-8"?>
Review comment:
> Does it make sense to make gremlin-test component to be a test
dependency for grenmlin-core? I am not a big fun of duplicating things
(including test),
I agree...we have a fair bit of duplication unfortunately and it drives me
crazy but there is at least some reason for it. Your instinct is right in that
it seems like `gremlin-core` should depend on `gremlin-test` but that would
create a circular dependency. `gremlin-test` actually depends on `gremlin-core`
currently because it is the technology test kit that providers use to validate
their implementations. we technically need a shared base test module for a
small handful of things that could be shared with `gremlin-test`,
`gremlin-core` (and below that `gremlin-language`). I've resisted adding that
because the handful of things is just small enough to make adding a new module
unappetizing and as you say, adds a bit more complexity to things.
There are some things happening now with the refactoring of the technology
test kit that might make a restructuring more necessary and appealing, but for
the immediate term I'd say that we're a bit stuck with some duplication
here/there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
> Support default attribute values in GraphMLReader
> -------------------------------------------------
>
> Key: TINKERPOP-848
> URL: https://issues.apache.org/jira/browse/TINKERPOP-848
> Project: TinkerPop
> Issue Type: Improvement
> Components: io
> Affects Versions: 3.0.2-incubating
> Reporter: Pavel Klinov
> Priority: Trivial
> Original Estimate: 2h
> Remaining Estimate: 2h
>
> Looking at the code of GraphMLReader I see that it doesn't support default
> values of attributes, which are allowed by the GraphML spec. This is a bit
> annoying especially if the input defines default values for attributes which
> are used for mandatory data, e.g. edge labels.
> One small example is the sample graph at [1]. "d_e" is the label attribute
> with a default value. There're <edge .. /> elements w/o body later in the
> document and reading those will throw a "java.lang.IllegalArgumentException:
> Label can not be null" exception (if the vendor considers edge labels
> mandatory).
> I'd personaly squash both keyIdMap and keyTypesMap into a single String ->
> AttrInfo map, where AttrInfo would contain information about the data
> attribute name, type, and the default value.
> [1] http://www.eecs.wsu.edu/~yyao/DirectedStudyI/Datasets/AS/sample.graphml
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
