You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@lucene.apache.org by moshebla <gi...@git.apache.org> on 2018/06/05 12:12:26 UTC
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
GitHub user moshebla opened a pull request:
https://github.com/apache/lucene-solr/pull/395
WIP SOLR-12362: add tests for working relational docs
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/moshebla/lucene-solr SOLR-12362-docsasval
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/lucene-solr/pull/395.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #395
----
commit e725d7c5a8bdcc65ea031bb4036e788388fc9af0
Author: user <us...@...>
Date: 2018-06-05T12:11:52Z
SOLR-12362: add tests for working relational docs
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193783399
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
--- End diff --
the comment is concerning; we don't want JsonRecordReader to do that anymore?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193532042
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -661,13 +664,13 @@ private Object parseSingleFieldValue(int ev, SolrInputField sif) throws IOExcept
}
}
- private boolean isChildDoc(Map<String, Object> extendedMap) {
+ private boolean isChildDoc(SolrInputDocument extendedMap) {
return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
}
- private Map<String, Object> generateExtendedValueMap(int ev) throws IOException {
+ private SolrInputDocument generateExtendedValueMap(int ev) throws IOException {
--- End diff --
this method name is no longer appropriate
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194768590
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +572,71 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
private void parseExtendedFieldValue(SolrInputField sif, int ev) throws IOException {
assert ev == JSONParser.OBJECT_START;
- Object normalFieldValue = null;
- Map<String, Object> extendedInfo = null;
+ SolrInputDocument extendedSolrDocument = generateExtendedValueDoc(ev);
- for (; ; ) {
- ev = parser.nextEvent();
- switch (ev) {
- case JSONParser.STRING:
- String label = parser.getString();
- if ("boost".equals(label)) {
- ev = parser.nextEvent();
- if (ev != JSONParser.NUMBER &&
- ev != JSONParser.LONG &&
- ev != JSONParser.BIGNUMBER) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Boost should have number. "
- + "Unexpected " + JSONParser.getEventString(ev) + " at [" + parser.getPosition() + "], field=" + sif.getName());
- }
+ if (isChildDoc(extendedSolrDocument)) {
+ SolrInputDocument cDoc = new SolrInputDocument();
+ for (Map.Entry<String, SolrInputField> extendedEntry: extendedSolrDocument.entrySet()) {
+ cDoc.setField(extendedEntry.getKey(), extendedEntry.getValue().getValue());
+ }
+ sif.addValue(cDoc);
+ return;
+ }
- String message = "Ignoring field boost: " + parser.getDouble() + " as index-time boosts are not supported anymore";
- if (WARNED_ABOUT_INDEX_TIME_BOOSTS.compareAndSet(false, true)) {
- log.warn(message);
- } else {
- log.debug(message);
- }
- } else if ("value".equals(label)) {
- normalFieldValue = parseNormalFieldValue(parser.nextEvent(), sif.getName());
- } else {
- // If we encounter other unknown map keys, then use a map
- if (extendedInfo == null) {
- extendedInfo = new HashMap<>(2);
- }
- // for now, the only extended info will be field values
- // we could either store this as an Object or a SolrInputField
- Object val = parseNormalFieldValue(parser.nextEvent(), sif.getName());
- extendedInfo.put(label, val);
- }
- break;
+ Object normalFieldValue = null;
+ Map<String, Object> extendedInfo = null;
- case JSONParser.OBJECT_END:
- if (extendedInfo != null) {
- if (normalFieldValue != null) {
- extendedInfo.put("value", normalFieldValue);
- }
- sif.setValue(extendedInfo);
- } else {
- sif.setValue(normalFieldValue);
- }
- return;
+ for (String label: extendedSolrDocument.keySet() ) {
--- End diff --
Whenever I see looping over keys from a map-like thing, I often see a line of code that then fetches the value, as seen here. This internally results in a bunch of lookups that aren't necessary. Instead, note that SolrInputDocument is Iterable<SolrInputField> (which itself has the key and value), so just loop over that.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195122269
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
True, didn't quite notice it, I'll upload a commit soon. :smile:
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194445760
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
ok so you are trying to retain the semantic information that the input was an array (potentially multi-valued) even if the particular input doc only had one value. Maybe say more or that, hinting at why we even care. (why do we care)? For regular field values, we don't at this level -- the schema holds multiValue info so it's dealth with later. Again I'm liking my suggestion of putting a virtual child doc field in the schema as it addresses the desire to know this semantic info, plus I think it may add some consistency (avoids special case you're doing here)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194761432
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -664,8 +672,38 @@ private Object parseSingleFieldValue(int ev, String fieldName) throws IOExceptio
if (ev == JSONParser.ARRAY_END) {
return lst;
}
- Object val = parseSingleFieldValue(ev, fieldName);
+ Object val = parseSingleFieldValue(ev, sif);
lst.add(val);
+ sif.setValue(null);
+ }
+ }
+
+ private boolean isChildDoc(SolrInputDocument extendedMap) {
+ return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
+ }
+
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
+ assert ev == JSONParser.OBJECT_START;
+ SolrInputDocument extendedInfo = new SolrInputDocument();
+
+ while(true) {
+ ev = parser.nextEvent();
+ if (ev == JSONParser.OBJECT_END) {
+ return extendedInfo;
+ }
+ String label = parser.getString();
+ SolrInputField sif = new SolrInputField(label);
+ parseFieldValue(sif);
+ extendedInfo.addField(label, sif.getValue());
--- End diff --
I think you can use extendedInfo.putField(key,SolrInputField) here? Oh I see comments elsewhere in this file explaining why addField is used; maybe copy-paste those comments?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Is it OK to close this, since SOLR-12362 was resolved?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194444902
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -668,7 +682,40 @@ private boolean isChildDoc(SolrInputDocument extendedMap) {
return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
}
- private SolrInputDocument generateExtendedValueMap(int ev) throws IOException {
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private void safeAddValue(SolrInputDocument doc, String fieldName, Object value) {
--- End diff --
Okay I see. Interesting -- there are clearly pros/cons to having SolrInputDocument implement core JDK abstractions. It makes working with it easier, though now some things like what you see here happens automatically when we don't want it to.
There is no perfect solution here but I'd rather see SolrInputField.addValue do an instanceof check when it sees that `v` implements Iterable to guard against iterating the child doc. This is much smaller/simpler than your safeAddValue, and it's something that won't be tied to Json or any other input format. Even someone who wants to use SolrJ and thinks that they can just call set/addValue would be surprised it doesn't work unless we make the change at SolrInputField.
Also admittedly, this is a gotcha to the path of putting nested child docs in values, but not a big deal.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Looks good!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193785664
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -668,7 +682,40 @@ private boolean isChildDoc(SolrInputDocument extendedMap) {
return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
}
- private SolrInputDocument generateExtendedValueMap(int ev) throws IOException {
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private void safeAddValue(SolrInputDocument doc, String fieldName, Object value) {
--- End diff --
I'm confused why this is necessary. What goes wrong if doc.addField(name,value) is called? If something goes wrong and this is necessary, that explanation needs to be in the comments.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194765026
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -664,8 +672,38 @@ private Object parseSingleFieldValue(int ev, String fieldName) throws IOExceptio
if (ev == JSONParser.ARRAY_END) {
return lst;
}
- Object val = parseSingleFieldValue(ev, fieldName);
+ Object val = parseSingleFieldValue(ev, sif);
lst.add(val);
+ sif.setValue(null);
+ }
+ }
+
+ private boolean isChildDoc(SolrInputDocument extendedMap) {
+ return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
+ }
+
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
--- End diff --
the name seems off -- 'generate' isn't bad but this method is parsing and plenty of other methods here parse and are named as such so I think this should be `parseExtendedValueAsDoc` or better `parseExtendedFieldValueAsDoc` since it is only called by `parseExtendedFieldValue`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194245516
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -668,7 +682,40 @@ private boolean isChildDoc(SolrInputDocument extendedMap) {
return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
}
- private SolrInputDocument generateExtendedValueMap(int ev) throws IOException {
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private void safeAddValue(SolrInputDocument doc, String fieldName, Object value) {
--- End diff --
I tried the same method, except because SolrInputDocument implements iterable, each document key is added, instead of the SolrInputDocument object. This chain of event occurs because [SolrInputDocument.addField](https://github.com/moshebla/lucene-solr/blob/3c51c1c414c6f7cb359e3ef442ca706e33cf3a7a/solr/solrj/src/java/org/apache/solr/common/SolrInputDocument.java#L84) calls [SolrInputField.addValue](https://github.com/moshebla/lucene-solr/blob/3c51c1c414c6f7cb359e3ef442ca706e33cf3a7a/solr/solrj/src/java/org/apache/solr/common/SolrInputField.java#L91).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Yes. Oddly enough,I don’t have permissions to close PRs unless I remember to mention it inthe commit message.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193217572
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +556,105 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
+ private Map<String, Object> generateExtendedValueMap(int ev) throws IOException {
+ assert ev == JSONParser.OBJECT_START;
+ Map<String, Object> extendedInfo = new HashMap<>();
+
+ for(; ; ) {
+ ev = parser.nextEvent();
+ if (ev == JSONParser.OBJECT_END) {
+ return extendedInfo;
+ }
+ String label = parser.getString();
+ SolrInputField sif = new SolrInputField(label);
+ parseFieldValue(sif);
+ extendedInfo.put(label, sif.getValue());
+ }
+ }
+
+ private boolean isChildDoc(Map<String, Object> extendedMap) {
+ if (extendedMap.containsKey("value") && extendedMap.containsKey("boost")) {
--- End diff --
lets instead simply check for the uniqueKey label
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193789756
--- Diff: solr/solrj/src/java/org/apache/solr/common/util/JsonRecordReader.java ---
@@ -438,18 +442,19 @@ void walkObject() throws IOException {
}
}
- private void addChildDoc2ParentDoc(Map<String, Object> record, Map<String, Object> values) {
+ private void addChildDoc2ParentDoc(Map<String, Object> record, Map<String, Object> values, String path) {
+ String trimmedPath = trimPath(path);
--- End diff --
I think "path" should be renamed "key", and the trimPath() should be done by the caller, and not be called trimPath either for that matter -- it takes the "leaf" or "suffix" part of the path, or perhaps you have an idea of a better name.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195098134
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -570,37 +573,34 @@ private SolrInputDocument parseDoc(int ev) throws IOException {
private void parseFieldValue(SolrInputField sif) throws IOException {
int ev = parser.nextEvent();
if (ev == JSONParser.OBJECT_START) {
- parseExtendedFieldValue(sif, ev);
+ parseExtendedFieldValue(ev, sif);
} else {
Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
- private void parseExtendedFieldValue(SolrInputField sif, int ev) throws IOException {
+ private void parseExtendedFieldValue(int ev, SolrInputField sif) throws IOException {
assert ev == JSONParser.OBJECT_START;
- SolrInputDocument extendedSolrDocument = generateExtendedValueDoc(ev);
+ SolrInputDocument extendedSolrDocument = parseExtendedValueAsDoc(ev);
if (isChildDoc(extendedSolrDocument)) {
- SolrInputDocument cDoc = new SolrInputDocument();
- for (Map.Entry<String, SolrInputField> extendedEntry: extendedSolrDocument.entrySet()) {
- cDoc.setField(extendedEntry.getKey(), extendedEntry.getValue().getValue());
- }
- sif.addValue(cDoc);
+ sif.addValue(extendedSolrDocument);
return;
}
Object normalFieldValue = null;
Map<String, Object> extendedInfo = null;
- for (String label: extendedSolrDocument.keySet() ) {
- Object fieldVal = extendedSolrDocument.get(label).getValue();
- if ("boost".equals(label)) {
+ for (SolrInputField field: extendedSolrDocument) {
+ Object fieldVal = field.getValue();
+ String fieldName = field.getName();
--- End diff --
Just tackled this one.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195093742
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
It's actually quite refreshing to get another person's opinion.
I don't think it would work flawlessly without changing a lot of the code, since buildDoc expects a Map as an input, so we would have to parse the map beforehand.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194760654
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +572,71 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
private void parseExtendedFieldValue(SolrInputField sif, int ev) throws IOException {
--- End diff --
Lets put "ev" as first arg to be more consistent
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194690146
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
if we wanted to build the original JSON document as it was indexed, we would have to store it as a single valued array if that was the case. I'm afraid I did not quite get what you meant by using virtual fields, what for?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194761077
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +572,71 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
private void parseExtendedFieldValue(SolrInputField sif, int ev) throws IOException {
assert ev == JSONParser.OBJECT_START;
- Object normalFieldValue = null;
- Map<String, Object> extendedInfo = null;
+ SolrInputDocument extendedSolrDocument = generateExtendedValueDoc(ev);
- for (; ; ) {
- ev = parser.nextEvent();
- switch (ev) {
- case JSONParser.STRING:
- String label = parser.getString();
- if ("boost".equals(label)) {
- ev = parser.nextEvent();
- if (ev != JSONParser.NUMBER &&
- ev != JSONParser.LONG &&
- ev != JSONParser.BIGNUMBER) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Boost should have number. "
- + "Unexpected " + JSONParser.getEventString(ev) + " at [" + parser.getPosition() + "], field=" + sif.getName());
- }
+ if (isChildDoc(extendedSolrDocument)) {
+ SolrInputDocument cDoc = new SolrInputDocument();
+ for (Map.Entry<String, SolrInputField> extendedEntry: extendedSolrDocument.entrySet()) {
+ cDoc.setField(extendedEntry.getKey(), extendedEntry.getValue().getValue());
--- End diff --
Why can't you simply do sif.addValue(extendedSolrDocument)? Thus avoiding the rebuilding of a SolrInputDocument that seems to have no purpose.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193217004
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +556,105 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
+ private Map<String, Object> generateExtendedValueMap(int ev) throws IOException {
+ assert ev == JSONParser.OBJECT_START;
+ Map<String, Object> extendedInfo = new HashMap<>();
+
+ for(; ; ) {
--- End diff --
nitpick: I prefer while(true)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193216812
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -556,82 +556,105 @@ private void parseFieldValue(SolrInputField sif) throws IOException {
if (ev == JSONParser.OBJECT_START) {
parseExtendedFieldValue(sif, ev);
} else {
- Object val = parseNormalFieldValue(ev, sif.getName());
+ Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
+ private Map<String, Object> generateExtendedValueMap(int ev) throws IOException {
--- End diff --
Can you please sequence the methods so this follows where it's called from so readers needn't look back up?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Hopefully these new commits address the problems with the previous changes
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195071571
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -570,37 +573,34 @@ private SolrInputDocument parseDoc(int ev) throws IOException {
private void parseFieldValue(SolrInputField sif) throws IOException {
int ev = parser.nextEvent();
if (ev == JSONParser.OBJECT_START) {
- parseExtendedFieldValue(sif, ev);
+ parseExtendedFieldValue(ev, sif);
} else {
Object val = parseNormalFieldValue(ev, sif);
sif.setValue(val);
}
}
- private void parseExtendedFieldValue(SolrInputField sif, int ev) throws IOException {
+ private void parseExtendedFieldValue(int ev, SolrInputField sif) throws IOException {
assert ev == JSONParser.OBJECT_START;
- SolrInputDocument extendedSolrDocument = generateExtendedValueDoc(ev);
+ SolrInputDocument extendedSolrDocument = parseExtendedValueAsDoc(ev);
if (isChildDoc(extendedSolrDocument)) {
- SolrInputDocument cDoc = new SolrInputDocument();
- for (Map.Entry<String, SolrInputField> extendedEntry: extendedSolrDocument.entrySet()) {
- cDoc.setField(extendedEntry.getKey(), extendedEntry.getValue().getValue());
- }
- sif.addValue(cDoc);
+ sif.addValue(extendedSolrDocument);
return;
}
Object normalFieldValue = null;
Map<String, Object> extendedInfo = null;
- for (String label: extendedSolrDocument.keySet() ) {
- Object fieldVal = extendedSolrDocument.get(label).getValue();
- if ("boost".equals(label)) {
+ for (SolrInputField field: extendedSolrDocument) {
+ Object fieldVal = field.getValue();
+ String fieldName = field.getName();
--- End diff --
I can understand why you chose the names `fieldName` and `fieldVal` because you are getting this from a SolrInputField. But recognize what's going on here is very unusual ... we are at this point treating this SolrInputDocument as if it were a map and not as a document. Lets add comments about this very explicitly before we loop because this is so strange. Maybe the `field` var could be named `entry`. The _former_ name `label` (or perhaps `extLabel`) var name is much more appropriate as it is _not_ a treated as *field* label/name. Likewise, just choose `val` or `value` or `extValue`.
Lets also add a bit of docs to parseExtendedFieldValue explain what this is for, since it's very non-obvious, I think. Essentially, this is a JSON object that is _either_ a child doc, or it's a Map (used for atomic updates), or it's an outdated mechanism to specify the boost which is no longer in Lucene/Solr.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193786291
--- Diff: solr/core/src/test/org/apache/solr/handler/JsonLoaderTest.java ---
@@ -435,61 +435,80 @@ public void testJsonDocFormat() throws Exception{
public void testFewParentsJsonDoc() throws Exception {
String json = PARENT_TWO_CHILDREN_JSON;
+ assertFewParents(json, true);
+ tearDown();
--- End diff --
I'm don't think I've ever seen a test do this, and I don't think we should start now. Simply add two test methods, one that passes true, one that passes false. test methods need to be named differently of course. This pattern in testing is pretty common.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Tried tackling the javadoc references, as I'm fairly new this I hope I didn't mess it up too badly.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195131919
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
It worked.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195082392
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
Good. The duplication of comment now has me wondering about this method overall -- why does it seem to be duplicating existing logic -- buildDoc? Would things "just work" if we replaced generateExtendedValueDoc with buildDoc()? I think it would? Less look-a-like code to maintain. I suppose the only problem with that is it would permit weird things like anonymous child docs on a labelled child doc. I just wouldn't worry about that now though; we catch that later in AddUpdateCommand.flatten().
BTW I'm sure you realize now that I don't have everything figured out from the start; it's an iterative journey for you and me/the-reviewer. :-)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194755084
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
Any way; lets not increase the scope of this particular issue with my idea to add this info to the schema. It seems adequate to force the use of an internal array for a single value in SolrInputField that we know originated from an array.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr issue #395: WIP SOLR-12362: add tests for working relational doc...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on the issue:
https://github.com/apache/lucene-solr/pull/395
Skimmed through, making some last minute changes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193216315
--- Diff: solr/core/src/test/org/apache/solr/handler/JsonLoaderTest.java ---
@@ -946,5 +946,87 @@ public void testGrandChildDocs() throws Exception {
}
+ @Test
+ public void testRelationalChildDocs() throws Exception {
--- End diff --
A bit on terminology.... if so-called "relational" child docs are going to be the only way to have a child doc in the future, then lets just start calling these simply child docs without the "relational" qualifier. "Relational" may be kinda confusing since an anonymous relationship is still related. Perhaps the new way is a "labelled" child doc? If you disagree and want to keep some qualifier then okay. If you see an existing test that says "child doc" for anonymous child doc then change its name to be about "anonymous" child docs.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla closed the pull request at:
https://github.com/apache/lucene-solr/pull/395
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195118329
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -593,14 +602,14 @@ private void parseExtendedFieldValue(int ev, SolrInputField sif) throws IOExcept
Object normalFieldValue = null;
Map<String, Object> extendedInfo = null;
- for (SolrInputField field: extendedSolrDocument) {
- Object fieldVal = field.getValue();
- String fieldName = field.getName();
- if ("boost".equals(fieldName)) {
- Object boostVal = fieldVal;
+ for (SolrInputField entry: extendedSolrDocument) {
+ Object label = entry.getValue();
--- End diff --
oops
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195119625
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
Sorry I meant `parseDoc`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194766692
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -664,8 +672,38 @@ private Object parseSingleFieldValue(int ev, String fieldName) throws IOExceptio
if (ev == JSONParser.ARRAY_END) {
return lst;
}
- Object val = parseSingleFieldValue(ev, fieldName);
+ Object val = parseSingleFieldValue(ev, sif);
lst.add(val);
+ sif.setValue(null);
+ }
+ }
+
+ private boolean isChildDoc(SolrInputDocument extendedMap) {
--- End diff --
rename extendedMap to extendedFieldValue
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194446507
--- Diff: solr/solrj/src/java/org/apache/solr/common/util/JsonRecordReader.java ---
@@ -442,19 +442,18 @@ void walkObject() throws IOException {
}
}
- private void addChildDoc2ParentDoc(Map<String, Object> record, Map<String, Object> values, String path) {
- String trimmedPath = trimPath(path);
+ private void addChildDoc2ParentDoc(Map<String, Object> record, Map<String, Object> values, String key) {
--- End diff --
much better; thanks
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194754268
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
By "virtual field" I'm referring to my earlier idea about having a special field in the schema that designates a child document with this relation. it would have a name, multiValued option, and probably that's it. https://issues.apache.org/jira/browse/SOLR-12298?focusedCommentId=16467523&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16467523
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194245379
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
--- End diff --
I changed the dictionary implementation to insert child documents into the map under their key, instead of null, so their relation to the parent document can be saved and used when SolrInputDocument is constructed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195118056
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -703,6 +703,10 @@ private SolrInputDocument generateExtendedValueDoc(int ev) throws IOException {
String label = parser.getString();
SolrInputField sif = new SolrInputField(label);
parseFieldValue(sif);
+ // pulling out the pieces may seem weird, but it's because
--- End diff --
Skimming through the code on master, it looks as if the map parsing was done here before only partially. Currently it seems like we either duplicate the map parsing part, or the document parsing part. If we go with the map part, we would build an intermediate map and then parse it to a document instead of streaming it using the parser. I'm not quite sure if using buildDoc would be as beneficial, since in either case, we have to duplicate some part.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194245569
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
It can handle it internally, but we have to ensure a JSON list is added as a list of one document, and not as a single document, to remain true to the original JSON document.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r193783038
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -249,14 +251,27 @@ public void handle(Map<String, Object> record, String path) {
private SolrInputDocument buildDoc(Map<String, Object> m) {
SolrInputDocument result = new SolrInputDocument();
for (Map.Entry<String, Object> e : m.entrySet()) {
- if (e.getKey() == null) {// special case. JsonRecordReader emits child docs with null key
+ if (entryIsChildDoc(e.getValue())) {// special case. JsonRecordReader emits child docs with null key
if (e.getValue() instanceof List) {
List value = (List) e.getValue();
for (Object o : value) {
- if (o instanceof Map) result.addChildDocument(buildDoc((Map) o));
+ if (o instanceof Map) {
+ if (anonChildDocFlag) {
+ result.addChildDocument(buildDoc((Map) o));
+ } else {
+ if(!result.containsKey(e.getKey())) {
+ result.setField(e.getKey(), new ArrayList<>(1));
--- End diff --
This looks unnecessary; SolrInputDocument internally handles ensuring multiple values can be captures.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by dsmiley <gi...@git.apache.org>.
Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r195116663
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -593,14 +602,14 @@ private void parseExtendedFieldValue(int ev, SolrInputField sif) throws IOExcept
Object normalFieldValue = null;
Map<String, Object> extendedInfo = null;
- for (SolrInputField field: extendedSolrDocument) {
- Object fieldVal = field.getValue();
- String fieldName = field.getName();
- if ("boost".equals(fieldName)) {
- Object boostVal = fieldVal;
+ for (SolrInputField entry: extendedSolrDocument) {
+ Object label = entry.getValue();
--- End diff --
LOL you have these mixed up :-) entry.getName should be the label.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org
[GitHub] lucene-solr pull request #395: WIP SOLR-12362: add tests for working relatio...
Posted by moshebla <gi...@git.apache.org>.
Github user moshebla commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/395#discussion_r194689072
--- Diff: solr/core/src/java/org/apache/solr/handler/loader/JsonLoader.java ---
@@ -668,7 +682,40 @@ private boolean isChildDoc(SolrInputDocument extendedMap) {
return extendedMap.containsKey(req.getSchema().getUniqueKeyField().getName());
}
- private SolrInputDocument generateExtendedValueMap(int ev) throws IOException {
+ private boolean entryIsChildDoc(Object val) {
+ if(val instanceof List) {
+ List listVal = (List) val;
+ if (listVal.size() == 0) return false;
+ return listVal.get(0) instanceof Map;
+ }
+ return val instanceof Map;
+ }
+
+ private void safeAddValue(SolrInputDocument doc, String fieldName, Object value) {
--- End diff --
So if I understood correctly, I also thought of checking to see if the addField value is of SolrDocumentBase, to prevent going through the iterator. I was afraid of backwards compatibility, but after some thought, until [SOLR-12361](https://issues.apache.org/jira/browse/SOLR-12361) childDocuments inside doc keys was not supported.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org