mdmarshmallow commented on code in PR #841: URL: https://github.com/apache/lucene/pull/841#discussion_r875205786
########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,163 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.BinaryDocValues; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.search.DocIdSetIterator; + +/** Get counts given a list of HyperRectangles (which must be of the same type) */ +public class HyperRectangleFacetCounts extends Facets { + /** Hypper rectangles passed to constructor. */ + protected final HyperRectangle[] hyperRectangles; + + /** Counts, initialized in by subclass. */ + protected final int[] counts; + + /** Our field name. */ + protected final String field; + + /** Number of dimensions for field */ + protected final int dims; + + /** Total number of hits. */ + protected int totCount; + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of long hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, LongHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of double hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, DoubleHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + private HyperRectangleFacetCounts( + boolean discarded, String field, FacetsCollector hits, HyperRectangle... hyperRectangles) Review Comment: Nothing really, I just wanted to make all the `HyperRectangle`'s be of the same subclass, though we could also leave it up to the user to decide whether they want that or not, in which case I could just do `HyperRectangle...` ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangle.java: ########## @@ -0,0 +1,46 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +/** Holds the name and the number of dims for a HyperRectangle */ +public abstract class HyperRectangle { + /** Label that identifies this range. */ + public final String label; + + /** How many dimensions this hyper rectangle has (IE: a regular rectangle would have dims=2) */ + public final int dims; + + /** Sole constructor. */ + protected HyperRectangle(String label, int dims) { + if (label == null) { + throw new IllegalArgumentException("label must not be null"); + } + if (dims <= 0) { + throw new IllegalArgumentException("Dims must be greater than 0. Dims=" + dims); + } + this.label = label; + this.dims = dims; + } + + /** + * Converts hyper rectangles ranges into a comparable long from whatever type it is in + * + * @param dim dimension of the request range + * @return The comparable long version of the requested range + */ + public abstract LongHyperRectangle.LongRangePair getComparableDimRange(int dim); Review Comment: Moved the `LongRangePair` to `HyperRectangle` and made concrete implementation of `getComparableDimRange`. I ended up making a few code changes as a result of this but I think it is cleaner now. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/DoubleHyperRectangle.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import org.apache.lucene.util.NumericUtils; + +/** Stores a hyper rectangle as an array of DoubleRangePairs */ +public class DoubleHyperRectangle extends HyperRectangle { + + /** Stores pair as LongRangePair */ + private final LongHyperRectangle.LongRangePair[] pairs; + + /** Created DoubleHyperRectangle */ Review Comment: Typo, thanks for catching! ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.BinaryDocValues; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.search.DocIdSetIterator; + +/** Get counts given a list of HyperRectangles (which must be of the same type) */ +public class HyperRectangleFacetCounts extends Facets { + /** Hypper rectangles passed to constructor. */ + protected final HyperRectangle[] hyperRectangles; + + /** Counts, initialized in subclass. */ + protected final int[] counts; + + /** Our field name. */ + protected final String field; + + /** Number of dimensions for field */ + protected final int dims; + + /** Total number of hits. */ + protected int totCount; + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of long hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, LongHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of double hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, DoubleHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + private HyperRectangleFacetCounts( + boolean discarded, String field, FacetsCollector hits, HyperRectangle... hyperRectangles) + throws IOException { + assert hyperRectangles.length > 0 : "Hyper rectangle ranges cannot be empty"; + this.field = field; + this.hyperRectangles = hyperRectangles; + this.dims = hyperRectangles[0].dims; + assert isHyperRectangleDimsConsistent() + : "All hyper rectangles must be the same dimensionality"; + this.counts = new int[hyperRectangles.length]; + count(field, hits.getMatchingDocs()); + } + + private boolean isHyperRectangleDimsConsistent() { + for (HyperRectangle hyperRectangle : hyperRectangles) { Review Comment: Didn't realize `allMatch` was a thing, thanks for the suggestion! ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/DoublePointFacetField.java: ########## @@ -0,0 +1,47 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import org.apache.lucene.document.BinaryDocValuesField; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.util.NumericUtils; + +/** + * Takes an array of doubles and converts them to sortable longs, then stores as a {@link + * BinaryDocValuesField} + */ +public class DoublePointFacetField extends BinaryDocValuesField { + + /** + * Creates a new DoublePointFacetField, indexing the provided N-dimensional long point. + * + * @param name field name + * @param point double[] value + * @throws IllegalArgumentException if the field name or value is null. + */ + public DoublePointFacetField(String name, double... point) { + super(name, LongPoint.pack(convertToSortableLongPoint(point))); + } + + private static long[] convertToSortableLongPoint(double[] point) { + long[] ret = new long[point.length]; Review Comment: Agree it makes the code look a lot cleaner :) ########## lucene/core/src/java/org/apache/lucene/document/LongPoint.java: ########## @@ -117,6 +117,25 @@ public static BytesRef pack(long... point) { return new BytesRef(packed); } + /** + * Unpack a BytesRef into a long point + * + * @param bytesRef BytesRef Value + * @throws IllegalArgumentException the value is null + */ + public static long[] unpack(BytesRef bytesRef) { Review Comment: If I understand correctly, are you suggesting to decode each dimension, and then check each dim against all the hyperrectangles before moving on to the next dim to do the same thing? I think that makes tracking the counts harder as the counts are per hyperrectangle. Let me know if I am misunderstanding this though. For now I'll keep it the way it is. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.BinaryDocValues; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.search.DocIdSetIterator; + +/** Get counts given a list of HyperRectangles (which must be of the same type) */ +public class HyperRectangleFacetCounts extends Facets { + /** Hypper rectangles passed to constructor. */ + protected final HyperRectangle[] hyperRectangles; + + /** Counts, initialized in subclass. */ + protected final int[] counts; + + /** Our field name. */ + protected final String field; + + /** Number of dimensions for field */ + protected final int dims; + + /** Total number of hits. */ + protected int totCount; + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of long hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, LongHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of double hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, DoubleHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + private HyperRectangleFacetCounts( + boolean discarded, String field, FacetsCollector hits, HyperRectangle... hyperRectangles) + throws IOException { + assert hyperRectangles.length > 0 : "Hyper rectangle ranges cannot be empty"; + this.field = field; + this.hyperRectangles = hyperRectangles; + this.dims = hyperRectangles[0].dims; + assert isHyperRectangleDimsConsistent() Review Comment: Changed in next revision. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/DoubleHyperRectangle.java: ########## @@ -0,0 +1,90 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import org.apache.lucene.util.NumericUtils; + +/** Stores a hyper rectangle as an array of DoubleRangePairs */ +public class DoubleHyperRectangle extends HyperRectangle { + + /** Stores pair as LongRangePair */ + private final LongHyperRectangle.LongRangePair[] pairs; + + /** Created DoubleHyperRectangle */ + public DoubleHyperRectangle(String label, DoubleRangePair... pairs) { + super(label, checkPairsAndGetDim(pairs)); + this.pairs = new LongHyperRectangle.LongRangePair[pairs.length]; + for (int dim = 0; dim < pairs.length; dim++) { + long longMin = NumericUtils.doubleToSortableLong(pairs[dim].min); + long longMax = NumericUtils.doubleToSortableLong(pairs[dim].max); + this.pairs[dim] = new LongHyperRectangle.LongRangePair(longMin, true, longMax, true); Review Comment: I think passing `true` here always is fine since `min` and `max` are always inclusive themselves, the boolean values just determine whether the params provided are inclusive or not, but if exclusive they are changed to become inclusive. That being said, I think introducing a `toLongRangePair` function makes sense and will make the code easier to read. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,175 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.lucene.facet.hyperrectangle; + +import java.io.IOException; +import java.util.Collections; +import java.util.List; +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.BinaryDocValues; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.search.DocIdSetIterator; + +/** Get counts given a list of HyperRectangles (which must be of the same type) */ +public class HyperRectangleFacetCounts extends Facets { + /** Hypper rectangles passed to constructor. */ + protected final HyperRectangle[] hyperRectangles; + + /** Counts, initialized in subclass. */ + protected final int[] counts; + + /** Our field name. */ + protected final String field; + + /** Number of dimensions for field */ + protected final int dims; + + /** Total number of hits. */ + protected int totCount; + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of long hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, LongHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + /** + * Create HyperRectangleFacetCounts using + * + * @param field Field name + * @param hits Hits to facet on + * @param hyperRectangles List of double hyper rectangle facets + * @throws IOException If there is a problem reading the field + */ + public HyperRectangleFacetCounts( + String field, FacetsCollector hits, DoubleHyperRectangle... hyperRectangles) + throws IOException { + this(true, field, hits, hyperRectangles); + } + + private HyperRectangleFacetCounts( + boolean discarded, String field, FacetsCollector hits, HyperRectangle... hyperRectangles) + throws IOException { + assert hyperRectangles.length > 0 : "Hyper rectangle ranges cannot be empty"; + this.field = field; + this.hyperRectangles = hyperRectangles; + this.dims = hyperRectangles[0].dims; + assert isHyperRectangleDimsConsistent() + : "All hyper rectangles must be the same dimensionality"; + this.counts = new int[hyperRectangles.length]; + count(field, hits.getMatchingDocs()); + } + + private boolean isHyperRectangleDimsConsistent() { + for (HyperRectangle hyperRectangle : hyperRectangles) { + if (hyperRectangle.dims != this.dims) { + return false; + } + } + return true; + } + + /** Counts from the provided field. */ + private void count(String field, List<FacetsCollector.MatchingDocs> matchingDocs) + throws IOException { + + for (int i = 0; i < matchingDocs.size(); i++) { + + FacetsCollector.MatchingDocs hits = matchingDocs.get(i); + + BinaryDocValues binaryDocValues = DocValues.getBinary(hits.context.reader(), field); + + final DocIdSetIterator it = hits.bits.iterator(); + if (it == null) { + continue; + } + + for (int doc = it.nextDoc(); doc != DocIdSetIterator.NO_MORE_DOCS; ) { + if (binaryDocValues.advanceExact(doc)) { + long[] point = LongPoint.unpack(binaryDocValues.binaryValue()); + assert point.length == dims + : "Point dimension (dim=" + + point.length + + ") is incompatible with hyper rectangle dimension (dim=" + + dims + + ")"; + // linear scan, change this to use R trees + boolean docIsValid = false; + for (int j = 0; j < hyperRectangles.length; j++) { + boolean validPoint = true; + for (int dim = 0; dim < dims; dim++) { + LongHyperRectangle.LongRangePair range = + hyperRectangles[j].getComparableDimRange(dim); + if (!range.accept(point[dim])) { + validPoint = false; + break; + } + } + if (validPoint) { + counts[j]++; + docIsValid = true; + } + } + if (docIsValid) { + totCount++; + } + } + doc = it.nextDoc(); + } + } + } + + @Override + public FacetResult getTopChildren(int topN, String dim, String... path) throws IOException { + validateTopN(topN); + if (dim.equals(field) == false) { Review Comment: Changed in next revision -- 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: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
