amatiushkin commented on a change in pull request #1485:
URL: https://github.com/apache/tinkerpop/pull/1485#discussion_r731510822
##########
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:
> Is there a reason you couldn't cache these defaults when they are
first parsed at GraphMLTokens.DEFAULT?
Empty tag and self-closing tag will cause exception for any type except
string, see test **noDefaultIntValue**.
Consider key of integer type: `<key id="age" for="node" attr.name="age"
attr.type="int">`
```java
case GraphMLTokens.DATA:
//...
String value = reader.getElementText(); // will return empty string
//...
typeCastValue(key, value, keyTypesMaps) // will generate exception,
if key type is integer
```
So,if you want me to move parsing for default values under `END_ELEMENT`, I
have to move parsing of none-default values as well.
I don't mind doing it, but this refactoring would make sense with other
change, e.g. to use push-down automata instead of holding individual cached
values to preserve parsing context.
So far I am trying to minimize my changes and reduce impact radius.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
