fabriziofortino commented on code in PR #649:
URL: https://github.com/apache/jackrabbit-oak/pull/649#discussion_r949328519
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java:
##########
@@ -142,32 +148,39 @@ protected boolean
isFulltextValuePersistedAtNode(PropertyDefinition pd) {
@Override
protected void indexTypedProperty(ElasticDocument doc, PropertyState
property, String pname, PropertyDefinition pd, int i) {
- // Get the Type tag from the defined index definition here - and not
from the actual persisted property state - this way in case
- // If the actual property value is different from the property type
defined in the index definition/mapping - this will try to convert the property
if possible,
- // otherwise will log a warning and not try and add the property to
index. If we try and index incompatible data types (like String to Date),
- // we would get an exception while indexing the node on elastic search
and other properties for the node will also don't get indexed. (See OAK-9665).
- int tag = pd.getType();
- Object f;
+ // Try to index the value as we receive it from the user. Elastic will
try to coerce the value to the type defined
+ // in the index. If this fails, the ES index is configured to ignore
the malformed fields (see ElasticIndexHelper)
+ // and continue indexing the document. The only exception are fields
of type boolean, because ES does not support
+ // ignoring malformed values for boolean types.
+
+ int pdTypeTag = pd.getType();
try {
- if (tag == Type.LONG.tag()) {
- f = property.getValue(Type.LONG, i);
- } else if (tag == Type.DATE.tag()) {
- f = property.getValue(Type.DATE, i);
- } else if (tag == Type.DOUBLE.tag()) {
- f = property.getValue(Type.DOUBLE, i);
- } else if (tag == Type.BOOLEAN.tag()) {
+ Object f;
+ if (pdTypeTag == Type.BOOLEAN.tag()) {
+ // Try to convert to boolean here, as ES does not support
ignore_malformed in boolean fields
f = property.getValue(Type.BOOLEAN, i).toString();
} else {
- f = property.getValue(Type.STRING, i);
+ // Let ES convert the property and rely on
ignore_malformed=true to skip if the value is not valid
+ int indexTypeTag = property.getType().tag();
Review Comment:
cant we still use `pdTypeTag` here?
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticIndexHelper.java:
##########
@@ -194,6 +197,18 @@ private static void mapInternalProperties(XContentBuilder
mappingBuilder) throws
// .endObject();
}
+ private static void createNestedAnalyzed(XContentBuilder mappingBuilder)
throws IOException {
Review Comment:
I would change the name to something like `createAnalyzedSubfield`. We
should not use `Nested` since it can be confused with the nested type in
elasticsearch that we use widely but not in this case.
##########
oak-core/src/test/java/org/apache/jackrabbit/oak/query/AbstractQueryTest.java:
##########
@@ -344,13 +345,21 @@ protected static void assertResultSorted(@NotNull
List<String> expected, @NotNul
assertEquals("Expected sorted result not found", expected, actual);
}
+ /**
+ * Checks that the two lists have the same elements ignoring their order.
+ */
protected static void assertResult(@NotNull List<String> expected,
@NotNull List<String> actual) {
- for (String p : checkNotNull(expected)) {
- assertTrue("Expected path " + p + " not found, got " + actual,
checkNotNull(actual)
- .contains(p));
- }
- assertEquals("Result set size is different: " + actual,
expected.size(),
- actual.size());
+ checkNotNull(expected);
+ checkNotNull(actual);
+ assertEquals("Result set size is different. Expected: " + expected +
", actual: " + actual, expected.size(), actual.size());
+
+ String[] sortedActual = actual.toArray(new String[0]);
+ Arrays.sort(sortedActual);
+
+ String[] sortedExpected = expected.toArray(new String[0]);
+ Arrays.sort(sortedExpected);
+
+ assertTrue("Invalid result set. Expected: " + expected + ", Actual: "
+ actual, Arrays.equals(sortedExpected, sortedActual));
Review Comment:
this can be simplified
```suggestion
assertArrayEquals("Invalid result set. Expected: " + expected + ",
Actual: " + actual, sortedExpected, sortedActual);
```
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java:
##########
@@ -142,32 +148,39 @@ protected boolean
isFulltextValuePersistedAtNode(PropertyDefinition pd) {
@Override
protected void indexTypedProperty(ElasticDocument doc, PropertyState
property, String pname, PropertyDefinition pd, int i) {
- // Get the Type tag from the defined index definition here - and not
from the actual persisted property state - this way in case
- // If the actual property value is different from the property type
defined in the index definition/mapping - this will try to convert the property
if possible,
- // otherwise will log a warning and not try and add the property to
index. If we try and index incompatible data types (like String to Date),
- // we would get an exception while indexing the node on elastic search
and other properties for the node will also don't get indexed. (See OAK-9665).
- int tag = pd.getType();
- Object f;
+ // Try to index the value as we receive it from the user. Elastic will
try to coerce the value to the type defined
+ // in the index. If this fails, the ES index is configured to ignore
the malformed fields (see ElasticIndexHelper)
+ // and continue indexing the document. The only exception are fields
of type boolean, because ES does not support
+ // ignoring malformed values for boolean types.
+
+ int pdTypeTag = pd.getType();
try {
- if (tag == Type.LONG.tag()) {
- f = property.getValue(Type.LONG, i);
- } else if (tag == Type.DATE.tag()) {
- f = property.getValue(Type.DATE, i);
- } else if (tag == Type.DOUBLE.tag()) {
- f = property.getValue(Type.DOUBLE, i);
- } else if (tag == Type.BOOLEAN.tag()) {
+ Object f;
+ if (pdTypeTag == Type.BOOLEAN.tag()) {
+ // Try to convert to boolean here, as ES does not support
ignore_malformed in boolean fields
f = property.getValue(Type.BOOLEAN, i).toString();
} else {
- f = property.getValue(Type.STRING, i);
+ // Let ES convert the property and rely on
ignore_malformed=true to skip if the value is not valid
+ int indexTypeTag = property.getType().tag();
+ if (indexTypeTag == Type.LONG.tag()) {
+ f = property.getValue(Type.LONG, i);
+ } else if (indexTypeTag == Type.DATE.tag()) {
+ f = property.getValue(Type.DATE, i);
+ } else if (indexTypeTag == Type.DOUBLE.tag()) {
+ f = property.getValue(Type.DOUBLE, i);
+ } else if (indexTypeTag == Type.BOOLEAN.tag()) {
+ f = property.getValue(Type.BOOLEAN, i).toString();
+ } else {
Review Comment:
is this needed?
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/index/ElasticDocumentMaker.java:
##########
@@ -142,32 +148,39 @@ protected boolean
isFulltextValuePersistedAtNode(PropertyDefinition pd) {
@Override
protected void indexTypedProperty(ElasticDocument doc, PropertyState
property, String pname, PropertyDefinition pd, int i) {
- // Get the Type tag from the defined index definition here - and not
from the actual persisted property state - this way in case
- // If the actual property value is different from the property type
defined in the index definition/mapping - this will try to convert the property
if possible,
- // otherwise will log a warning and not try and add the property to
index. If we try and index incompatible data types (like String to Date),
- // we would get an exception while indexing the node on elastic search
and other properties for the node will also don't get indexed. (See OAK-9665).
- int tag = pd.getType();
- Object f;
+ // Try to index the value as we receive it from the user. Elastic will
try to coerce the value to the type defined
+ // in the index. If this fails, the ES index is configured to ignore
the malformed fields (see ElasticIndexHelper)
+ // and continue indexing the document. The only exception are fields
of type boolean, because ES does not support
+ // ignoring malformed values for boolean types.
+
+ int pdTypeTag = pd.getType();
try {
- if (tag == Type.LONG.tag()) {
- f = property.getValue(Type.LONG, i);
- } else if (tag == Type.DATE.tag()) {
- f = property.getValue(Type.DATE, i);
- } else if (tag == Type.DOUBLE.tag()) {
- f = property.getValue(Type.DOUBLE, i);
- } else if (tag == Type.BOOLEAN.tag()) {
+ Object f;
+ if (pdTypeTag == Type.BOOLEAN.tag()) {
+ // Try to convert to boolean here, as ES does not support
ignore_malformed in boolean fields
f = property.getValue(Type.BOOLEAN, i).toString();
} else {
- f = property.getValue(Type.STRING, i);
+ // Let ES convert the property and rely on
ignore_malformed=true to skip if the value is not valid
+ int indexTypeTag = property.getType().tag();
+ if (indexTypeTag == Type.LONG.tag()) {
+ f = property.getValue(Type.LONG, i);
Review Comment:
the comment above says `Let ES convert the property ...` but here (and also
for the other types below) we do `property.getValue(Type.LONG, i);`. Doesn't
this call perform a conversion? If not, we should at least put a comment here
because I find it confusing.
--
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]
