gsmiller commented on code in PR #841: URL: https://github.com/apache/lucene/pull/841#discussion_r877627338
########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/DoubleHyperRectangle.java: ########## @@ -0,0 +1,88 @@ +/* + * 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.util.Arrays; +import org.apache.lucene.util.NumericUtils; + +/** Stores a hyper rectangle as an array of DoubleRangePairs */ +public class DoubleHyperRectangle extends HyperRectangle { Review Comment: As sort of a blanket comment for all the new classes you created, I might suggest we add `@lucene.experimental`. This is a pretty big, new bit of work, and I could see us wanting to tweak and change this a bit here-and-there. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/DoubleHyperRectangle.java: ########## @@ -0,0 +1,88 @@ +/* + * 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.util.Arrays; +import org.apache.lucene.util.NumericUtils; + +/** Stores a hyper rectangle as an array of DoubleRangePairs */ +public class DoubleHyperRectangle extends HyperRectangle { + + /** Creates DoubleHyperRectangle */ + public DoubleHyperRectangle(String label, DoubleRangePair... pairs) { + super(label, convertToLongRangePairs(pairs)); + } + + private static LongRangePair[] convertToLongRangePairs(DoubleRangePair... pairs) { + if (pairs == null || pairs.length == 0) { + throw new IllegalArgumentException("Pairs cannot be null or empty"); + } + return Arrays.stream(pairs).map(DoubleRangePair::toLongRangePair).toArray(LongRangePair[]::new); + } + + /** Defines a single range in a DoubleHyperRectangle */ + public static class DoubleRangePair { + /** Inclusive min */ + public final double min; + + /** Inclusive max */ + public final double max; + + /** + * Creates a DoubleRangePair, very similar to the constructor of {@link + * org.apache.lucene.facet.range.DoubleRange} + * + * @param minIn Min value of pair + * @param minInclusive If minIn is inclusive + * @param maxIn Max value of pair + * @param maxInclusive If maxIn is inclusive + */ + public DoubleRangePair(double minIn, boolean minInclusive, double maxIn, boolean maxInclusive) { + if (Double.isNaN(minIn) || Double.isNaN(maxIn)) { + throw new IllegalArgumentException( + "min and max cannot be NaN: min=" + minIn + ", max=" + maxIn); + } + + if (!minInclusive) { + minIn = Math.nextUp(minIn); + } + + if (!maxInclusive) { + maxIn = Math.nextDown(maxIn); + } + + if (minIn > maxIn) { + throw new IllegalArgumentException( + "Minimum cannot be greater than maximum, max=" + maxIn + ", min=" + minIn); + } + + this.min = minIn; + this.max = maxIn; + } + + /** + * Converts this to a LongRangePair with sortable long equivalents + * + * @return A LongRangePair equivalent of this object + */ + public LongRangePair toLongRangePair() { Review Comment: Does this need to be public? I think it's only used internally in `DoubleHyperRectangle` right? Should we reduce visibility (unless we expect users need this functionality directly?). ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,149 @@ +/* + * 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.Arrays; +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; Review Comment: I noticed you made all these fields `protected`. Were you thinking this might be a useful extension point for users? I might recommend against that, at least for now. If users start extending this, it might limit the changes we can make going forward (needing to stay back-compat with some of our internal implementation details). ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangle.java: ########## @@ -0,0 +1,101 @@ +/* + * 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 { Review Comment: Can this be made pkg-private? ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/LongPointFacetField.java: ########## @@ -0,0 +1,35 @@ +/* + * 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; + +/** Packs an array of longs into a {@link BinaryDocValuesField} */ +public class LongPointFacetField extends BinaryDocValuesField { Review Comment: I was thinking about these field classes you created a little more, and I'm wondering if we should create something more generic than just for faceting? I think what you've may run into here is the fact that we don't actually have a field type for indexing point values as doc values (that I know of anyway). We have all the `xxxPoint` fields for adding inverted fields in the points index (e.g., `LongPoint`), but I don't think we have an actual representation for adding them as DVs. What do you think of moving these into the `document` package and actually defining them as general DV field types? We might not have to go so far as to actually formalize this new concept in the `DocValuesType` enum with their own format and such. Under the hood, they could just be a binary format like you have here (at least to start). You might look at `LongRangeDocValuesField` as a good example of what I mean. ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangleFacetCounts.java: ########## @@ -0,0 +1,149 @@ +/* + * 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.Arrays; +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. */ Review Comment: typo? -- 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
