nfsantos commented on code in PR #2145:
URL: https://github.com/apache/jackrabbit-oak/pull/2145#discussion_r1984627358
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/ElasticIndexDefinition.java:
##########
@@ -324,13 +325,17 @@ public String getElasticKeyword(String propertyName) {
ElasticPropertyDefinition pd =
getMatchingRegexPropertyDefinition(propertyName);
if (pd != null) {
if (pd.isFlattened()) {
- return FieldNames.FLATTENED_FIELD_PREFIX + pd.nodeName +
"." + propertyName;
+ String fieldName =
ElasticIndexUtils.fieldName(propertyName);
+ String flattenedFieldName =
FieldNames.FLATTENED_FIELD_PREFIX +
+ ElasticIndexUtils.fieldName(pd.nodeName) + "." +
fieldName;
+ return flattenedFieldName;
}
}
- return propertyName + ".keyword";
+ String fieldName = ElasticIndexUtils.fieldName(propertyName);
+ return fieldName + ".keyword";
}
- String field = propertyName;
+ String field = ElasticIndexUtils.fieldName(propertyName);
Review Comment:
Again a repeated call to `ElasticIndexUtils.fieldName`
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtils.java:
##########
@@ -78,10 +103,9 @@ public static List<Float> toFloats(byte[] array) {
* @return byte array
*/
public static byte[] toByteArray(List<Float> values) {
- int blockSize = Float.SIZE / Byte.SIZE;
- byte[] bytes = new byte[values.size() * blockSize];
+ byte[] bytes = new byte[values.size() * Float.BYTES];
ByteBuffer wrap = ByteBuffer.wrap(bytes);
- for (int i = 0, j = 0; i < values.size(); i++, j += blockSize) {
+ for (int i = 0; i < values.size(); i++) {
Review Comment:
Nice!
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/ElasticRequestHandler.java:
##########
@@ -649,11 +650,14 @@ private ObjectBuilder<BoolQuery>
inference(BoolQuery.Builder b, String propertyN
private Stream<NestedQuery> dynamicScoreQueries(String text) {
return
elasticIndexDefinition.getDynamicBoostProperties().stream().map(pd ->
NestedQuery.of(n -> n
- .path(pd.nodeName)
+ .path(ElasticIndexUtils.fieldName(pd.nodeName))
.query(q -> q.functionScore(s -> s
.boost(DYNAMIC_BOOST_WEIGHT)
- .query(fq -> fq.match(m -> m.field(pd.nodeName +
".value").query(FieldValue.of(text))))
- .functions(f -> f.fieldValueFactor(fv ->
fv.field(pd.nodeName + ".boost")))))
+ .query(fq -> fq.match(m -> m.field(
+ ElasticIndexUtils.fieldName(pd.nodeName) +
".value").
+ query(FieldValue.of(text))))
+ .functions(f -> f.fieldValueFactor(fv -> fv.field(
+ ElasticIndexUtils.fieldName(pd.nodeName) +
".boost")))))
Review Comment:
Repeated calls to `ElasticIndexUtils.fieldName(pd.nodeName)` in the same
method. Call only once at the start of the `map()` handler.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtils.java:
##########
@@ -31,6 +31,31 @@ public class ElasticIndexUtils {
private static final Logger LOG =
LoggerFactory.getLogger(ElasticIndexUtils.class);
+ /**
+ * Convert a JCR property name to a Elasticsearch field name.
+ * Notice that "|" is not allowed in JCR names.
+ *
+ * "." is converted to "|dot|"
+ * "/" is converted to "||"
+ *
+ * @param propertyName the property name
+ * @return the field name
+ */
+ public static String fieldName(String propertyName) {
+ String fieldName = propertyName;
+ // 99% property names don't contain a slash or dot,
+ // so for performance reason use indexOf
+ int slashIndex = fieldName.indexOf('|');
+ if (slashIndex >= 0) {
+ fieldName = fieldName.replaceAll("\\|", "\\|\\|");
+ }
+ int dotIndex = fieldName.indexOf('.');
+ if (dotIndex >= 0) {
+ fieldName = fieldName.replaceAll("\\.", "|dot|");
+ }
+ return fieldName;
Review Comment:
This method is likely to be on the critical path for performance, so it
would likely be better to avoid replaceAll, and instead use a StringBuilder and
a simple loop over all the chars, after a first check to ensure that the String
does not contain any of the characters to be replaced.
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/query/async/facets/ElasticStatisticalFacetAsyncProvider.java:
##########
@@ -74,11 +75,15 @@ public class ElasticStatisticalFacetAsyncProvider
implements ElasticFacetProvide
this.elasticResponseHandler = elasticResponseHandler;
this.isAccessible = isAccessible;
- this.facetFields =
elasticRequestHandler.facetFields().collect(Collectors.toSet());
+ Set<String> elasticFieldNames = elasticRequestHandler.facetFields().
+ map(p -> ElasticIndexUtils.fieldName(p)).
+ collect(Collectors.toSet());
+ this.facetFields = elasticRequestHandler.facetFields().
+ collect(Collectors.toSet());
Review Comment:
Can't this be replaced with
`Set.copyOf(elasticRequestHandler.facetFields())`?
##########
oak-search-elastic/src/main/java/org/apache/jackrabbit/oak/plugins/index/elastic/util/ElasticIndexUtils.java:
##########
@@ -31,6 +31,31 @@ public class ElasticIndexUtils {
private static final Logger LOG =
LoggerFactory.getLogger(ElasticIndexUtils.class);
+ /**
+ * Convert a JCR property name to a Elasticsearch field name.
+ * Notice that "|" is not allowed in JCR names.
+ *
+ * "." is converted to "|dot|"
+ * "/" is converted to "||"
+ *
+ * @param propertyName the property name
+ * @return the field name
+ */
+ public static String fieldName(String propertyName) {
+ String fieldName = propertyName;
+ // 99% property names don't contain a slash or dot,
+ // so for performance reason use indexOf
+ int slashIndex = fieldName.indexOf('|');
+ if (slashIndex >= 0) {
+ fieldName = fieldName.replaceAll("\\|", "\\|\\|");
+ }
Review Comment:
The code seems to be inconsistent with the javadocs. The docs say that the
method replaces "." and "/", but here the code is looking for "|". Which is
pipe, not slash, so the variable name is also not consistent.
--
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]
