Index: src/main/java/org/apache/jackrabbit/oak/plugins/index/reference/ReferenceIndex.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/plugins/index/reference/ReferenceIndex.java (revision 1656667) +++ src/main/java/org/apache/jackrabbit/oak/plugins/index/reference/ReferenceIndex.java (working copy) @@ -51,7 +51,7 @@ */ class ReferenceIndex implements QueryIndex { - private static ContentMirrorStoreStrategy STORE = new ContentMirrorStoreStrategy(); + private static final ContentMirrorStoreStrategy STORE = new ContentMirrorStoreStrategy(); @Override public String getIndexName() { @@ -70,24 +70,31 @@ return Double.POSITIVE_INFINITY; } for (PropertyRestriction pr : filter.getPropertyRestrictions()) { - if (pr.propertyType == REFERENCE - || pr.propertyType == WEAKREFERENCE) { + if (isEqualityRestrictionOnType(pr, REFERENCE) || + isEqualityRestrictionOnType(pr, WEAKREFERENCE)) { return 1; } } // not an appropriate index return POSITIVE_INFINITY; } + + private static boolean isEqualityRestrictionOnType(PropertyRestriction pr, int propertyType) { + if (pr.propertyType != propertyType) { + return false; + } + return pr.first != null && pr.first == pr.last; + } @Override public Cursor query(Filter filter, NodeState root) { for (PropertyRestriction pr : filter.getPropertyRestrictions()) { - if (pr.propertyType == REFERENCE) { + if (isEqualityRestrictionOnType(pr, REFERENCE)) { String uuid = pr.first.getValue(STRING); String name = pr.propertyName; return lookup(root, uuid, name, REF_NAME, filter); } - if (pr.propertyType == WEAKREFERENCE) { + if (isEqualityRestrictionOnType(pr, WEAKREFERENCE)) { String uuid = pr.first.getValue(STRING); String name = pr.propertyName; return lookup(root, uuid, name, WEAK_REF_NAME, filter); Index: src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java (revision 1656667) +++ src/main/java/org/apache/jackrabbit/oak/query/index/FilterImpl.java (working copy) @@ -20,7 +20,6 @@ import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; import java.util.Iterator; import java.util.List; import java.util.Map.Entry; @@ -30,6 +29,8 @@ import javax.annotation.Nullable; import javax.jcr.PropertyType; +import com.google.common.collect.ArrayListMultimap; +import com.google.common.collect.ListMultimap; import org.apache.jackrabbit.oak.api.PropertyValue; import org.apache.jackrabbit.oak.commons.PathUtils; import org.apache.jackrabbit.oak.query.QueryEngineSettings; @@ -85,8 +86,15 @@ private FullTextExpression fullTextConstraint; - private final HashMap propertyRestrictions = - new HashMap(); + /** + * The list of restrictions for each property. A restriction may be x=1. + *

+ * Each property may have multiple restrictions, which means all + * restrictions must apply, for example x=1 and x=2. For this case, only + * multi-valued properties match if it contains both the values 1 and 2. + */ + private final ListMultimap propertyRestrictions = + ArrayListMultimap.create(); /** * Only return distinct values. @@ -237,15 +245,24 @@ return propertyRestrictions.values(); } - /** - * Get the restriction for the given property, if any. - * - * @param propertyName the property name - * @return the restriction or null - */ @Override public PropertyRestriction getPropertyRestriction(String propertyName) { - return propertyRestrictions.get(propertyName); + List list = propertyRestrictions.get(propertyName); + if (list.isEmpty()) { + return null; + } else if (list.size() == 1) { + return list.get(0); + } + int bestSort = -1; + PropertyRestriction best = null; + for (PropertyRestriction x : list) { + int sort = x.sortOrder(); + if (sort > bestSort) { + bestSort = sort; + best = x; + } + } + return best; } public boolean testPath(String path) { @@ -274,46 +291,33 @@ // not restricted return; } - PropertyRestriction x = addRestricition(propertyName); - if (x.propertyType != PropertyType.UNDEFINED && x.propertyType != propertyType) { - // already restricted to another property type - always false - setAlwaysFalse(); + for (PropertyRestriction old : getPropertyRestrictions(propertyName)) { + if (old.propertyType != PropertyType.UNDEFINED && old.propertyType != propertyType) { + // already restricted to another property type - always false + setAlwaysFalse(); + return; + } } + PropertyRestriction x = new PropertyRestriction(); + x.propertyName = propertyName; x.propertyType = propertyType; + addRestriction(x); } public void restrictPropertyAsList(String propertyName, List list) { - PropertyRestriction x = addRestricition(propertyName); - if (x.list == null) { - x.list = list; - } else { - // this is required for multi-valued properties: - // for example, if a multi-value property p contains {1, 2}, - // and we search using "p in (1, 3) and p in (2, 4)", then - // this needs to match - so we search for "p in (1, 2, 3, 4)" - x.list.removeAll(list); - x.list.addAll(list); - } + PropertyRestriction x = new PropertyRestriction(); + x.propertyName = propertyName; + x.list = list; + addRestriction(x); } public void restrictProperty(String propertyName, Operator op, PropertyValue v) { - PropertyRestriction x = addRestricition(propertyName); - PropertyValue oldFirst = x.first; - PropertyValue oldLast = x.last; + PropertyRestriction x = new PropertyRestriction(); + x.propertyName = propertyName; switch (op) { case EQUAL: - if (x.first != null && x.last == x.first && x.firstIncluding && x.lastIncluding) { - // we keep the old equality condition if there is one; - // we can not use setAlwaysFalse, as this would not be correct - // for multi-valued properties: - // unlike in databases, "x = 1 and x = 2" can match a node - // if x is a multi-valued property with value {1, 2} - } else { - // all other conditions (range conditions) are replaced with this one - // (we can not use setAlwaysFalse for the same reason as above) - x.first = x.last = v; - x.firstIncluding = x.lastIncluding = true; - } + x.first = x.last = v; + x.firstIncluding = x.lastIncluding = true; break; case NOT_EQUAL: if (v != null) { @@ -321,59 +325,49 @@ } break; case GREATER_THAN: - // we don't narrow the range because of multi-valued properties - if (x.first == null) { - x.first = maxValue(oldFirst, v); - x.firstIncluding = false; - } + x.first = v; + x.firstIncluding = false; break; case GREATER_OR_EQUAL: - // we don't narrow the range because of multi-valued properties - if (x.first == null) { - x.first = maxValue(oldFirst, v); - x.firstIncluding = x.first == oldFirst ? x.firstIncluding : true; - } + x.first = v; + x.firstIncluding = true; break; case LESS_THAN: - // we don't narrow the range because of multi-valued properties - if (x.last == null) { - x.last = minValue(oldLast, v); - x.lastIncluding = false; - } + x.last = v; + x.lastIncluding = false; break; case LESS_OR_EQUAL: - // we don't narrow the range because of multi-valued properties - if (x.last == null) { - x.last = minValue(oldLast, v); - x.lastIncluding = x.last == oldLast ? x.lastIncluding : true; - } + x.last = v; + x.lastIncluding = true; break; case LIKE: - // we don't narrow the range because of multi-valued properties - if (x.first == null) { - // LIKE is handled in the fulltext index - x.isLike = true; - x.first = v; - } + // LIKE is handled in the fulltext index + x.isLike = true; + x.first = v; break; } - if (x.first != null && x.last != null) { - if (x.first.compareTo(x.last) > 0) { - setAlwaysFalse(); - } else if (x.first.compareTo(x.last) == 0 && (!x.firstIncluding || !x.lastIncluding)) { - setAlwaysFalse(); + addRestriction(x); + } + + /** + * Add a restriction for the given property, unless the exact same + * restriction is already set. + * + * @param restriction the restriction to add + */ + private void addRestriction(PropertyRestriction restriction) { + List list = getPropertyRestrictions(restriction.propertyName); + for (PropertyRestriction old : list) { + if (old.equals(restriction)) { + return; } } + list.add(restriction); } - private PropertyRestriction addRestricition(String propertyName) { - PropertyRestriction x = propertyRestrictions.get(propertyName); - if (x == null) { - x = new PropertyRestriction(); - x.propertyName = propertyName; - propertyRestrictions.put(propertyName, x); - } - return x; + @Override + public List getPropertyRestrictions(String propertyName) { + return propertyRestrictions.get(propertyName); } static PropertyValue maxValue(PropertyValue a, PropertyValue b) { @@ -406,10 +400,10 @@ buff.append(", path=").append(getPathPlan()); if (!propertyRestrictions.isEmpty()) { buff.append(", property=["); - Iterator> iterator = propertyRestrictions - .entrySet().iterator(); + Iterator>> iterator = propertyRestrictions + .asMap().entrySet().iterator(); while (iterator.hasNext()) { - Entry p = iterator.next(); + Entry> p = iterator.next(); buff.append(p.getKey()).append("=").append(p.getValue()); if (iterator.hasNext()) { buff.append(", "); Index: src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java =================================================================== --- src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java (revision 1656667) +++ src/main/java/org/apache/jackrabbit/oak/spi/query/Filter.java (working copy) @@ -52,8 +52,10 @@ SelectorImpl getSelector(); /** - * Get the list of property restrictions, if any. - * + * Get the list of property restrictions, if any. Each property may contain + * multiple restrictions, for example x=1 and x=2. For this case, only + * multi-valued properties match that contain both 1 and 2. + * * @return the conditions (an empty collection if not used) */ Collection getPropertyRestrictions(); @@ -83,12 +85,22 @@ boolean containsNativeConstraint(); /** - * Get the property restriction for the given property, if any. - * + * Get the most restrictive property restriction for the given property, if + * any. + * * @param propertyName the property name - * @return the restriction, or null if there is no restriction for this property + * @return the first restriction, or null if there is no restriction for + * this property */ PropertyRestriction getPropertyRestriction(String propertyName); + + /** + * Get the all property restriction for the given property. + * + * @param propertyName the property name + * @return the list of restrictions (possibly empty, never null) + */ + List getPropertyRestrictions(String propertyName); /** * Get the path restriction type. @@ -238,7 +250,100 @@ String li = last == null ? "" : (lastIncluding ? "]" : ")"); return fi + f + ".." + l + li; } + + /** + * How restrictive a condition is. + * + * @return 0 for "is not null", 10 for equality, and 5 for everything + * else + */ + public int sortOrder() { + if (first == null && last == null) { + if (list == null) { + return 0; + } else { + return 5; + } + } + if (first == last) { + return 10; + } + return 5; + } + + @Override + public int hashCode() { + // generated code (Eclipse) + final int prime = 31; + int result = 1; + result = prime * result + ((first == null) ? 0 : first.hashCode()); + result = prime * result + (firstIncluding ? 1231 : 1237); + result = prime * result + (isLike ? 1231 : 1237); + result = prime * result + ((last == null) ? 0 : last.hashCode()); + result = prime * result + (lastIncluding ? 1231 : 1237); + result = prime * result + ((list == null) ? 0 : list.hashCode()); + result = prime * result + + ((propertyName == null) ? 0 : propertyName.hashCode()); + result = prime * result + propertyType; + return result; + } + @Override + public boolean equals(Object obj) { + // generated code (Eclipse) + if (this == obj) { + return true; + } + if (obj == null) { + return false; + } + if (getClass() != obj.getClass()) { + return false; + } + PropertyRestriction other = (PropertyRestriction) obj; + if (first == null) { + if (other.first != null) { + return false; + } + } else if (!first.equals(other.first)) { + return false; + } + if (firstIncluding != other.firstIncluding) { + return false; + } + if (isLike != other.isLike) { + return false; + } + if (last == null) { + if (other.last != null) { + return false; + } + } else if (!last.equals(other.last)) { + return false; + } + if (lastIncluding != other.lastIncluding) { + return false; + } + if (list == null) { + if (other.list != null) { + return false; + } + } else if (!list.equals(other.list)) { + return false; + } + if (propertyName == null) { + if (other.propertyName != null) { + return false; + } + } else if (!propertyName.equals(other.propertyName)) { + return false; + } + if (propertyType != other.propertyType) { + return false; + } + return true; + } + } /** Index: src/test/java/org/apache/jackrabbit/oak/query/index/FilterTest.java =================================================================== --- src/test/java/org/apache/jackrabbit/oak/query/index/FilterTest.java (revision 1656667) +++ src/test/java/org/apache/jackrabbit/oak/query/index/FilterTest.java (working copy) @@ -45,56 +45,97 @@ FilterImpl f = new FilterImpl(); assertTrue(null == f.getPropertyRestriction("x")); f.restrictProperty("x", Operator.LESS_OR_EQUAL, two); - assertEquals("..2]", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[..2]]])", + f.toString()); f.restrictProperty("x", Operator.GREATER_OR_EQUAL, one); - assertEquals("[1..2]", f.getPropertyRestriction("x").toString()); - - // further narrowing will not change the restriction, - // to account for multi-valued properties + assertEquals( + "Filter(, path=*, property=[x=[..2], [1..]])", + f.toString()); + + // no change, as the same restrictions already were added + f.restrictProperty("x", Operator.LESS_OR_EQUAL, two); + assertEquals( + "Filter(, path=*, property=[x=[..2], [1..]])", + f.toString()); + f.restrictProperty("x", Operator.GREATER_OR_EQUAL, one); + assertEquals( + "Filter(, path=*, property=[x=[..2], [1..]])", + f.toString()); + f.restrictProperty("x", Operator.GREATER_THAN, one); - assertEquals("[1..2]", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[..2], [1.., (1..]])", + f.toString()); f.restrictProperty("x", Operator.LESS_THAN, two); - assertEquals("[1..2]", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[..2], [1.., (1.., ..2)]])", + f.toString()); - // this should replace the range with an equality - // (which is faster, and correct even when using multi-valued properties) + // TODO could replace / remove the old range conditions, + // if there is an overlap f.restrictProperty("x", Operator.EQUAL, two); - assertEquals("2", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[..2], [1.., (1.., ..2), 2]])", + f.toString()); f = new FilterImpl(); f.restrictProperty("x", Operator.EQUAL, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1]])", + f.toString()); f.restrictProperty("x", Operator.EQUAL, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1]])", + f.toString()); + + // TODO could replace / remove the old range conditions, + // if there is an overlap f.restrictProperty("x", Operator.GREATER_OR_EQUAL, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1, [1..]])", + f.toString()); f.restrictProperty("x", Operator.LESS_OR_EQUAL, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1, [1.., ..1]]])", + f.toString()); - // further narrowing will not change the restriction, - // to account for multi-valued properties + // TODO could replace / remove the old range conditions, + // if there is an overlap f.restrictProperty("x", Operator.GREATER_THAN, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1, [1.., ..1], (1..]])", + f.toString()); f = new FilterImpl(); f.restrictProperty("x", Operator.EQUAL, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1]])", + f.toString()); - // further narrowing will not change the restriction, - // to account for multi-valued properties + // TODO could replace / remove the old range conditions, + // if there is an overlap f.restrictProperty("x", Operator.LESS_THAN, one); - assertEquals("1", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[1, ..1)]])", + f.toString()); f = new FilterImpl(); f.restrictProperty("x", Operator.NOT_EQUAL, null); - assertEquals("", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[]])", + f.toString()); f.restrictProperty("x", Operator.LESS_THAN, one); - assertEquals("..1)", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[, ..1)]])", + f.toString()); // this should replace the range with an equality // (which is faster, and correct even when using multi-valued properties) f.restrictProperty("x", Operator.EQUAL, two); - assertEquals("2", f.getPropertyRestriction("x").toString()); + assertEquals( + "Filter(, path=*, property=[x=[, ..1), 2]])", + f.toString()); }