shaie commented on code in PR #841: URL: https://github.com/apache/lucene/pull/841#discussion_r878796006
########## lucene/core/src/java/org/apache/lucene/document/DoublePointDocValuesField.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.document; Review Comment: I am not opposed to this change, but I find it a bit strange that we add a "general" Point DV support without any tests that exercise it, and the only usage of it is in the Facet module. Do we see a use case in the future for other DV usage? Like Sorting? Anyway I'm fine either way, just wanted to comment here that since it's `@lucene.experimental` we could have also left it in the facet package and then move here if a more general use case came up. ########## lucene/facet/src/test/org/apache/lucene/facet/hyperrectangle/TestHyperRectangleFacetCounts.java: ########## @@ -0,0 +1,374 @@ +/* + * 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 org.apache.lucene.document.Document; +import org.apache.lucene.document.DoublePointDocValuesField; +import org.apache.lucene.document.LongPointDocValuesField; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.FacetTestCase; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.FacetsCollectorManager; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.store.Directory; +import org.apache.lucene.tests.index.RandomIndexWriter; + +public class TestHyperRectangleFacetCounts extends FacetTestCase { + + public void testBasicLong() throws Exception { + Directory d = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), d); + + for (long l = 0; l < 100; l++) { + Document doc = new Document(); + LongPointDocValuesField field = new LongPointDocValuesField("field", l, l + 1L, l + 2L); + doc.add(field); + w.addDocument(doc); + } + + // Also add point with Long.MAX_VALUE + Document doc = new Document(); + LongPointDocValuesField field = + new LongPointDocValuesField( + "field", Long.MAX_VALUE - 2L, Long.MAX_VALUE - 1L, Long.MAX_VALUE); + doc.add(field); + w.addDocument(doc); + + IndexReader r = w.getReader(); + w.close(); + + IndexSearcher s = newSearcher(r); + FacetsCollector fc = s.search(new MatchAllDocsQuery(), new FacetsCollectorManager()); + + Facets facets = + new HyperRectangleFacetCounts( + "field", + fc, + new LongHyperRectangle( + "less than (10, 11, 12)", + new HyperRectangle.LongRangePair(0L, true, 10L, false), + new HyperRectangle.LongRangePair(0L, true, 11L, false), + new HyperRectangle.LongRangePair(0L, true, 12L, false)), + new LongHyperRectangle( + "less than or equal to (10, 11, 12)", + new HyperRectangle.LongRangePair(0L, true, 10L, true), + new HyperRectangle.LongRangePair(0L, true, 11L, true), + new HyperRectangle.LongRangePair(0L, true, 12L, true)), + new LongHyperRectangle( + "over (90, 91, 92)", Review Comment: super nit: "Between (90,91,92) and (100,101,102)"? Cause two tests below we have "Over (1000...) which is really just Over, without a real upper limit. But feel free to ignore my pickiness :) ########## lucene/facet/src/test/org/apache/lucene/facet/hyperrectangle/TestHyperRectangleFacetCounts.java: ########## @@ -0,0 +1,374 @@ +/* + * 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 org.apache.lucene.document.Document; +import org.apache.lucene.document.DoublePointDocValuesField; +import org.apache.lucene.document.LongPointDocValuesField; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.FacetTestCase; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.FacetsCollectorManager; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.store.Directory; +import org.apache.lucene.tests.index.RandomIndexWriter; + +public class TestHyperRectangleFacetCounts extends FacetTestCase { + + public void testBasicLong() throws Exception { + Directory d = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), d); + + for (long l = 0; l < 100; l++) { + Document doc = new Document(); + LongPointDocValuesField field = new LongPointDocValuesField("field", l, l + 1L, l + 2L); + doc.add(field); + w.addDocument(doc); + } + + // Also add point with Long.MAX_VALUE + Document doc = new Document(); + LongPointDocValuesField field = + new LongPointDocValuesField( + "field", Long.MAX_VALUE - 2L, Long.MAX_VALUE - 1L, Long.MAX_VALUE); + doc.add(field); + w.addDocument(doc); + + IndexReader r = w.getReader(); + w.close(); + + IndexSearcher s = newSearcher(r); + FacetsCollector fc = s.search(new MatchAllDocsQuery(), new FacetsCollectorManager()); + + Facets facets = + new HyperRectangleFacetCounts( + "field", + fc, + new LongHyperRectangle( + "less than (10, 11, 12)", + new HyperRectangle.LongRangePair(0L, true, 10L, false), + new HyperRectangle.LongRangePair(0L, true, 11L, false), + new HyperRectangle.LongRangePair(0L, true, 12L, false)), + new LongHyperRectangle( + "less than or equal to (10, 11, 12)", + new HyperRectangle.LongRangePair(0L, true, 10L, true), + new HyperRectangle.LongRangePair(0L, true, 11L, true), + new HyperRectangle.LongRangePair(0L, true, 12L, true)), + new LongHyperRectangle( + "over (90, 91, 92)", + new HyperRectangle.LongRangePair(90L, false, 100L, false), + new HyperRectangle.LongRangePair(91L, false, 101L, false), + new HyperRectangle.LongRangePair(92L, false, 102L, false)), + new LongHyperRectangle( + "(90, 91, 92) or above", Review Comment: If you accept what I wrote above, then please change this too (and the double tests). ########## lucene/core/src/java/org/apache/lucene/document/LongPointDocValuesField.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.document; + +/** + * Packs an array of longs into a {@link BinaryDocValuesField} Review Comment: Can we make this jdoc consistent with the Double variant, mentioning that we're indexing Point values? ########## lucene/core/src/java/org/apache/lucene/document/LongPointDocValuesField.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.document; + +/** + * Packs an array of longs into a {@link BinaryDocValuesField} + * + * @lucene.experimental + */ +public class LongPointDocValuesField extends BinaryDocValuesField { + + /** + * Creates a new LongPointFacetField, indexing the provided N-dimensional long point. Review Comment: Same comment about `FacetField` ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangle.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.document.LongPoint; +import org.apache.lucene.util.ArrayUtil; + +/** + * Holds the label, the number of dims, and the point pairs for a HyperRectangle + * + * @lucene.experimental + */ +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; + + private final ArrayUtil.ByteArrayComparator byteComparator = + ArrayUtil.getUnsignedComparator(Long.BYTES); + + private final byte[] lowerPoints; + private final byte[] upperPoints; + + /** Sole constructor. */ + protected HyperRectangle(String label, LongRangePair... pairs) { + if (label == null) { + throw new IllegalArgumentException("label must not be null"); + } + if (pairs == null || pairs.length == 0) { + throw new IllegalArgumentException("Pairs cannot be null or empty"); + } + this.label = label; + this.dims = pairs.length; + + this.lowerPoints = + LongPoint.pack(Arrays.stream(pairs).mapToLong(pair -> pair.min).toArray()).bytes; + this.upperPoints = + LongPoint.pack(Arrays.stream(pairs).mapToLong(pair -> pair.max).toArray()).bytes; + } + + /** + * Checked a long packed value against this HyperRectangle. If you indexed a field with {@link + * org.apache.lucene.document.LongPointDocValuesField} or {@link + * org.apache.lucene.document.DoublePointDocValuesField}, those field values will be able to be Review Comment: s/will be able to/can/? ########## lucene/core/src/java/org/apache/lucene/document/DoublePointDocValuesField.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.document; + +import java.util.Arrays; +import org.apache.lucene.util.NumericUtils; + +/** + * Takes an array of doubles and converts them to sortable longs, then stores as a {@link + * BinaryDocValuesField} + * + * @lucene.experimental + */ +public class DoublePointDocValuesField extends BinaryDocValuesField { + + /** + * Creates a new DoublePointFacetField, indexing the provided N-dimensional long point. Review Comment: s/DoublePointFacetField/DoublePointDocValuesField/ s/long point/double point/ ########## lucene/facet/src/java/org/apache/lucene/facet/hyperrectangle/HyperRectangle.java: ########## @@ -0,0 +1,131 @@ +/* + * 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.document.LongPoint; +import org.apache.lucene.util.ArrayUtil; + +/** + * Holds the label, the number of dims, and the point pairs for a HyperRectangle + * + * @lucene.experimental + */ +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; + + private final ArrayUtil.ByteArrayComparator byteComparator = + ArrayUtil.getUnsignedComparator(Long.BYTES); + + private final byte[] lowerPoints; + private final byte[] upperPoints; + + /** Sole constructor. */ + protected HyperRectangle(String label, LongRangePair... pairs) { + if (label == null) { + throw new IllegalArgumentException("label must not be null"); + } + if (pairs == null || pairs.length == 0) { + throw new IllegalArgumentException("Pairs cannot be null or empty"); + } + this.label = label; + this.dims = pairs.length; + + this.lowerPoints = + LongPoint.pack(Arrays.stream(pairs).mapToLong(pair -> pair.min).toArray()).bytes; + this.upperPoints = + LongPoint.pack(Arrays.stream(pairs).mapToLong(pair -> pair.max).toArray()).bytes; + } + + /** + * Checked a long packed value against this HyperRectangle. If you indexed a field with {@link + * org.apache.lucene.document.LongPointDocValuesField} or {@link + * org.apache.lucene.document.DoublePointDocValuesField}, those field values will be able to be + * passed directly into this method. + * + * @param packedValue a byte array representing a long value + * @return whether the packed long point intersects with this HyperRectangle + */ + public final boolean matches(byte[] packedValue) { + assert packedValue.length / Long.BYTES == dims + : "Point dimension (dim=" + + packedValue.length / Long.BYTES + + ") is incompatible with hyper rectangle dimension (dim=" + + dims + + ")"; + for (int dim = 0; dim < dims; dim++) { Review Comment: Instead of iterating on `dim` you can iterate on `offset` starting from 0 to `packedValue.length` and increment by `Long.BYTES`? ########## lucene/core/src/java/org/apache/lucene/document/DoublePointDocValuesField.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.document; + +import java.util.Arrays; +import org.apache.lucene.util.NumericUtils; + +/** + * Takes an array of doubles and converts them to sortable longs, then stores as a {@link + * BinaryDocValuesField} Review Comment: nit: Maybe `A {@link BinaryDocValuesField} which indexes double point values as sortable-longs`? ########## lucene/facet/src/test/org/apache/lucene/facet/hyperrectangle/TestHyperRectangleFacetCounts.java: ########## @@ -0,0 +1,374 @@ +/* + * 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 org.apache.lucene.document.Document; +import org.apache.lucene.document.DoublePointDocValuesField; +import org.apache.lucene.document.LongPointDocValuesField; +import org.apache.lucene.facet.FacetResult; +import org.apache.lucene.facet.FacetTestCase; +import org.apache.lucene.facet.Facets; +import org.apache.lucene.facet.FacetsCollector; +import org.apache.lucene.facet.FacetsCollectorManager; +import org.apache.lucene.facet.LabelAndValue; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.store.Directory; +import org.apache.lucene.tests.index.RandomIndexWriter; + +public class TestHyperRectangleFacetCounts extends FacetTestCase { Review Comment: Two questions about the tests: 1. Would you like to add a test which verifies we assert that all given rectangles have the same dim? 2. Would you like to add a test which showcases mixed Long/Double rectangles? -- 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
