You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2022/05/21 00:36:02 UTC
[GitHub] [lucene] gsmiller opened a new pull request, #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
gsmiller opened a new pull request, #915:
URL: https://github.com/apache/lucene/pull/915
# Description
The facets module has had a fair amount of recent development, some resulting in a fair amount of copy/paste duplication. This attempts to clean it up a bit and simplify/refactor some of the internal functionality.
# Solution
Minor internal refactoring of some facet implementations and extraction of a common parent class to contain some duplicated logic in the SSDV faceting implementations.
# Tests
No new tests were written. All existing testing is still passing.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://github.com/apache/lucene/blob/main/CONTRIBUTING.md) and my code conforms to the standards described there to the best of my ability.
- [x] I have given Lucene maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on a diff in pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on code in PR #915:
URL: https://github.com/apache/lucene/pull/915#discussion_r879786369
##########
lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * 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.sortedset;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.PrimitiveIterator;
+import org.apache.lucene.facet.FacetResult;
+import org.apache.lucene.facet.Facets;
+import org.apache.lucene.facet.FacetsConfig;
+import org.apache.lucene.facet.FacetsConfig.DimConfig;
+import org.apache.lucene.facet.LabelAndValue;
+import org.apache.lucene.facet.TopOrdAndIntQueue;
+import org.apache.lucene.facet.sortedset.SortedSetDocValuesReaderState.DimTree;
+import org.apache.lucene.facet.sortedset.SortedSetDocValuesReaderState.OrdRange;
+import org.apache.lucene.index.SortedSetDocValues;
+import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.PriorityQueue;
+
+/** Base class for SSDV faceting implementations. */
+abstract class AbstractSortedSetDocValueFacetCounts extends Facets {
+
+ private static final Comparator<FacetResult> FACET_RESULT_COMPARATOR =
+ new Comparator<>() {
+ @Override
+ public int compare(FacetResult a, FacetResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return -1;
+ } else if (b.value.intValue() > a.value.intValue()) {
+ return 1;
+ } else {
+ return a.dim.compareTo(b.dim);
+ }
+ }
+ };
+
+ final SortedSetDocValuesReaderState state;
+ final FacetsConfig stateConfig;
+ final SortedSetDocValues dv;
+ final String field;
+
+ AbstractSortedSetDocValueFacetCounts(SortedSetDocValuesReaderState state) throws IOException {
+ this.state = state;
+ this.field = state.getField();
+ this.stateConfig = state.getFacetsConfig();
+ this.dv = state.getDocValues();
+ }
+
+ @Override
+ public FacetResult getTopChildren(int topN, String dim, String... path) throws IOException {
+ validateTopN(topN);
+ TopChildrenForPath topChildrenForPath = getTopChildrenForPath(topN, dim, path);
+ return createFacetResult(topChildrenForPath, dim, path);
+ }
+
+ @Override
+ public Number getSpecificValue(String dim, String... path) throws IOException {
+ if (path.length != 1) {
+ throw new IllegalArgumentException("path must be length=1");
+ }
+ int ord = (int) dv.lookupTerm(new BytesRef(FacetsConfig.pathToString(dim, path)));
+ if (ord < 0) {
+ return -1;
+ }
+
+ return getCount(ord);
+ }
+
+ @Override
+ public List<FacetResult> getAllDims(int topN) throws IOException {
+ validateTopN(topN);
+ List<FacetResult> results = new ArrayList<>();
+ for (String dim : state.getDims()) {
+ TopChildrenForPath topChildrenForPath = getTopChildrenForPath(topN, dim);
+ FacetResult facetResult = createFacetResult(topChildrenForPath, dim);
+ if (facetResult != null) {
+ results.add(facetResult);
+ }
+ }
+
+ // Sort by highest count:
+ results.sort(FACET_RESULT_COMPARATOR);
+ return results;
+ }
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ validateTopN(topNDims);
+ validateTopN(topNChildren);
+
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValue> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValue a, DimValue b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ // Keep track of intermediate results, if we compute them, so we can reuse them later:
+ Map<String, TopChildrenForPath> intermediateResults = null;
+
+ for (String dim : state.getDims()) {
+ DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ int dimCount;
+ if (dimConfig.hierarchical) {
+ int dimOrd = state.getDimTree(dim).dimStartOrd;
+ dimCount = getCount(dimOrd);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ int dimOrd = ordRange.start;
+ if (dimConfig.multiValued && dimConfig.requireDimCount) {
+ dimCount = getCount(dimOrd);
+ } else {
+ PrimitiveIterator.OfInt childIt = ordRange.iterator();
+ TopChildrenForPath topChildrenForPath =
+ computeTopChildren(childIt, topNChildren, dimConfig, dimOrd);
+ if (intermediateResults == null) {
+ intermediateResults = new HashMap<>();
+ }
+ intermediateResults.put(dim, topChildrenForPath);
+ dimCount = topChildrenForPath.pathCount;
+ }
+ }
+
+ if (dimCount != 0) {
+ if (pq.size() < topNDims) {
+ pq.add(new DimValue(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValue bottomDim = pq.top();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.updateTop();
+ }
+ }
+ }
+ }
+
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValue dimValue = pq.pop();
+ assert dimValue != null;
+ TopChildrenForPath topChildrenForPath = null;
+ if (intermediateResults != null) {
+ topChildrenForPath = intermediateResults.get(dimValue.dim);
+ }
+ if (topChildrenForPath == null) {
+ topChildrenForPath = getTopChildrenForPath(topNChildren, dimValue.dim);
+ }
+ FacetResult facetResult = createFacetResult(topChildrenForPath, dimValue.dim);
+ // should not be null since only dims with non-zero values were considered earlier
+ assert facetResult != null;
+ resultSize--;
+ results[resultSize] = facetResult;
+ }
+ return Arrays.asList(results);
+ }
+
+ abstract int getCount(int ord);
Review Comment:
nit: maybe add a javadoc here explaining if counts are aggregated at indexing time, we can just retrieve it.
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] mikemccand commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
mikemccand commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1138026556
Thank you @Yuti-G for running the dedicated `luceneutil` faceting benchmark!
But: the `getAllDims` time for SSDV seems to have gotten much faster with this PR, which is great! Was that expected? Or is this some horrible noise? Is it repeatable?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1135050190
Hi @gsmiller, thanks for making a lot of improvements to the code, and it looks great to me! I also ran the benchmarks for facet and do not observe much difference from the main branch. I added getTopDims to benchmarks but the PR hasn't merged yet, so the attached results are from my local. Thanks!
Main:
<img width="810" alt="Screen Shot 2022-05-23 at 11 12 26 AM" src="https://user-images.githubusercontent.com/44444710/169889777-cf059966-a38d-49b8-8699-e8ff5172967c.png">
pr/915:
<img width="830" alt="Screen Shot 2022-05-23 at 12 17 42 PM" src="https://user-images.githubusercontent.com/44444710/169890719-ec235cb4-4f49-47f9-9ca0-a1f822073bf5.png">
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1138932259
Since this change is purely meant to remove some code duplication and make some very minor optimizations, and doesn't modify the API or expose any additional API surface area, I plan to merge in the next couple of days unless anyone objects. If anyone wants more time to review or has feedback, I'm more than happy to wait. Thanks!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on a diff in pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on code in PR #915:
URL: https://github.com/apache/lucene/pull/915#discussion_r881201779
##########
lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java:
##########
@@ -0,0 +1,333 @@
+/*
+ * 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.sortedset;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Comparator;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.PrimitiveIterator;
+import org.apache.lucene.facet.FacetResult;
+import org.apache.lucene.facet.Facets;
+import org.apache.lucene.facet.FacetsConfig;
+import org.apache.lucene.facet.FacetsConfig.DimConfig;
+import org.apache.lucene.facet.LabelAndValue;
+import org.apache.lucene.facet.TopOrdAndIntQueue;
+import org.apache.lucene.facet.sortedset.SortedSetDocValuesReaderState.DimTree;
+import org.apache.lucene.facet.sortedset.SortedSetDocValuesReaderState.OrdRange;
+import org.apache.lucene.index.SortedSetDocValues;
+import org.apache.lucene.util.BytesRef;
+import org.apache.lucene.util.PriorityQueue;
+
+/** Base class for SSDV faceting implementations. */
+abstract class AbstractSortedSetDocValueFacetCounts extends Facets {
+
+ private static final Comparator<FacetResult> FACET_RESULT_COMPARATOR =
+ new Comparator<>() {
+ @Override
+ public int compare(FacetResult a, FacetResult b) {
+ if (a.value.intValue() > b.value.intValue()) {
+ return -1;
+ } else if (b.value.intValue() > a.value.intValue()) {
+ return 1;
+ } else {
+ return a.dim.compareTo(b.dim);
+ }
+ }
+ };
+
+ final SortedSetDocValuesReaderState state;
+ final FacetsConfig stateConfig;
+ final SortedSetDocValues dv;
+ final String field;
+
+ AbstractSortedSetDocValueFacetCounts(SortedSetDocValuesReaderState state) throws IOException {
+ this.state = state;
+ this.field = state.getField();
+ this.stateConfig = state.getFacetsConfig();
+ this.dv = state.getDocValues();
+ }
+
+ @Override
+ public FacetResult getTopChildren(int topN, String dim, String... path) throws IOException {
+ validateTopN(topN);
+ TopChildrenForPath topChildrenForPath = getTopChildrenForPath(topN, dim, path);
+ return createFacetResult(topChildrenForPath, dim, path);
+ }
+
+ @Override
+ public Number getSpecificValue(String dim, String... path) throws IOException {
+ if (path.length != 1) {
+ throw new IllegalArgumentException("path must be length=1");
+ }
+ int ord = (int) dv.lookupTerm(new BytesRef(FacetsConfig.pathToString(dim, path)));
+ if (ord < 0) {
+ return -1;
+ }
+
+ return getCount(ord);
+ }
+
+ @Override
+ public List<FacetResult> getAllDims(int topN) throws IOException {
+ validateTopN(topN);
+ List<FacetResult> results = new ArrayList<>();
+ for (String dim : state.getDims()) {
+ TopChildrenForPath topChildrenForPath = getTopChildrenForPath(topN, dim);
+ FacetResult facetResult = createFacetResult(topChildrenForPath, dim);
+ if (facetResult != null) {
+ results.add(facetResult);
+ }
+ }
+
+ // Sort by highest count:
+ results.sort(FACET_RESULT_COMPARATOR);
+ return results;
+ }
+
+ @Override
+ public List<FacetResult> getTopDims(int topNDims, int topNChildren) throws IOException {
+ validateTopN(topNDims);
+ validateTopN(topNChildren);
+
+ // Creates priority queue to store top dimensions and sort by their aggregated values/hits and
+ // string values.
+ PriorityQueue<DimValue> pq =
+ new PriorityQueue<>(topNDims) {
+ @Override
+ protected boolean lessThan(DimValue a, DimValue b) {
+ if (a.value > b.value) {
+ return false;
+ } else if (a.value < b.value) {
+ return true;
+ } else {
+ return a.dim.compareTo(b.dim) > 0;
+ }
+ }
+ };
+
+ // Keep track of intermediate results, if we compute them, so we can reuse them later:
+ Map<String, TopChildrenForPath> intermediateResults = null;
+
+ for (String dim : state.getDims()) {
+ DimConfig dimConfig = stateConfig.getDimConfig(dim);
+ int dimCount;
+ if (dimConfig.hierarchical) {
+ int dimOrd = state.getDimTree(dim).dimStartOrd;
+ dimCount = getCount(dimOrd);
+ } else {
+ OrdRange ordRange = state.getOrdRange(dim);
+ int dimOrd = ordRange.start;
+ if (dimConfig.multiValued && dimConfig.requireDimCount) {
+ dimCount = getCount(dimOrd);
+ } else {
+ PrimitiveIterator.OfInt childIt = ordRange.iterator();
+ TopChildrenForPath topChildrenForPath =
+ computeTopChildren(childIt, topNChildren, dimConfig, dimOrd);
+ if (intermediateResults == null) {
+ intermediateResults = new HashMap<>();
+ }
+ intermediateResults.put(dim, topChildrenForPath);
+ dimCount = topChildrenForPath.pathCount;
+ }
+ }
+
+ if (dimCount != 0) {
+ if (pq.size() < topNDims) {
+ pq.add(new DimValue(dim, dimCount));
+ } else {
+ if (dimCount > pq.top().value
+ || (dimCount == pq.top().value && dim.compareTo(pq.top().dim) < 0)) {
+ DimValue bottomDim = pq.top();
+ bottomDim.dim = dim;
+ bottomDim.value = dimCount;
+ pq.updateTop();
+ }
+ }
+ }
+ }
+
+ int resultSize = pq.size();
+ FacetResult[] results = new FacetResult[resultSize];
+
+ while (pq.size() > 0) {
+ DimValue dimValue = pq.pop();
+ assert dimValue != null;
+ TopChildrenForPath topChildrenForPath = null;
+ if (intermediateResults != null) {
+ topChildrenForPath = intermediateResults.get(dimValue.dim);
+ }
+ if (topChildrenForPath == null) {
+ topChildrenForPath = getTopChildrenForPath(topNChildren, dimValue.dim);
+ }
+ FacetResult facetResult = createFacetResult(topChildrenForPath, dimValue.dim);
+ // should not be null since only dims with non-zero values were considered earlier
+ assert facetResult != null;
+ resultSize--;
+ results[resultSize] = facetResult;
+ }
+ return Arrays.asList(results);
+ }
+
+ abstract int getCount(int ord);
Review Comment:
Good call. Thanks!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1138931375
@Yuti-G I just updated the PR with some additional comments/javadoc and a very minor optimization in the SSDV#getTopDims case. Could you have a look at the latest changes when you get a chance?
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1140406510
@Yuti-G this is merged onto `main` and backported to `branch_9x`. Please feel free to rebase your PR on top of it, and let me know if you have any questions. Also, thanks for mentioning your PR. I'd missed that but would like to have a look. I'll wait for you to rebase then I'll review. Thanks again (and hopefully rebasing isn't too big of a pain!)!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1138982348
Looks good to me! I will rebase my current work at https://github.com/apache/lucene/pull/914 - `getAllChildren` after this PR is merged. Thank you so much for making the code so clean!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller merged pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller merged PR #915:
URL: https://github.com/apache/lucene/pull/915
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1136707326
Thanks @Yuti-G for the feedback and benchmark results! I appreciate you taking a look since I know you're quite familiar with this code. I saw a couple opportunities to de-dupe some code, but you did all the hard work introducing the feature. Thanks again!
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] Yuti-G commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
Yuti-G commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1138850060
> But: the `getAllDims` time for SSDV seems to have gotten much faster with this PR, which is great! Was that expected? Or is this some horrible noise? Is it repeatable?
I think it's just noise. I just re-run the dedicated luceneutil faceting benchmark against the main branch:
1st run:
<img width="839" alt="Screen Shot 2022-05-26 at 9 55 35 AM" src="https://user-images.githubusercontent.com/44444710/170536870-929c6d0d-1d47-4fb3-bd80-c30e62c8d51e.png">
2nd run:
<img width="824" alt="Screen Shot 2022-05-26 at 9 56 07 AM" src="https://user-images.githubusercontent.com/44444710/170536986-492cf393-c3f7-4248-bdb7-60ceec0aa1e7.png">
3rd run:
<img width="889" alt="Screen Shot 2022-05-26 at 10 49 52 AM" src="https://user-images.githubusercontent.com/44444710/170546269-34be4600-e384-4e18-bd0f-cc7ac3936a6e.png">
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene] gsmiller commented on pull request #915: LUCENE-10585: Scrub copy/paste code in the facets module and attempt to simplify a bit
Posted by GitBox <gi...@apache.org>.
gsmiller commented on PR #915:
URL: https://github.com/apache/lucene/pull/915#issuecomment-1133483198
Ran benchmarks to make sure this didn't regress performance in any meaningful way. I'm not seeing any regressions:
```
TaskQPS baseline StdDevQPS candidate StdDev Pct diff p-value
Wildcard 337.70 (16.7%) 323.50 (16.7%) -4.2% ( -32% - 35%) 0.426
BrowseMonthTaxoFacets 29.43 (13.8%) 28.72 (19.1%) -2.4% ( -31% - 35%) 0.649
BrowseDateSSDVFacets 2.68 (17.5%) 2.63 (17.0%) -1.8% ( -30% - 39%) 0.743
BrowseDayOfYearSSDVFacets 14.94 (16.5%) 14.68 (13.6%) -1.8% ( -27% - 33%) 0.709
HighTermMonthSort 147.90 (22.4%) 145.50 (19.4%) -1.6% ( -35% - 51%) 0.807
HighTermDayOfYearSort 61.68 (14.9%) 60.80 (15.3%) -1.4% ( -27% - 33%) 0.766
OrHighNotHigh 1107.68 (4.1%) 1092.99 (2.8%) -1.3% ( -7% - 5%) 0.232
LowTerm 2181.38 (3.5%) 2153.14 (3.1%) -1.3% ( -7% - 5%) 0.213
OrHighNotMed 1420.79 (2.7%) 1404.11 (3.1%) -1.2% ( -6% - 4%) 0.203
TermDTSort 117.08 (15.5%) 115.77 (15.3%) -1.1% ( -27% - 35%) 0.818
BrowseRandomLabelTaxoFacets 18.20 (10.0%) 18.01 (13.2%) -1.1% ( -22% - 24%) 0.769
LowSloppyPhrase 121.57 (4.8%) 120.52 (3.7%) -0.9% ( -8% - 8%) 0.527
HighTerm 1427.75 (4.8%) 1416.29 (3.0%) -0.8% ( -8% - 7%) 0.529
MedSloppyPhrase 92.18 (5.1%) 91.45 (4.0%) -0.8% ( -9% - 8%) 0.589
BrowseDateTaxoFacets 20.98 (12.4%) 20.82 (15.5%) -0.7% ( -25% - 30%) 0.866
OrHighNotLow 1233.19 (4.3%) 1223.99 (3.8%) -0.7% ( -8% - 7%) 0.557
OrHighLow 515.20 (2.8%) 511.43 (2.7%) -0.7% ( -6% - 4%) 0.400
BrowseDayOfYearTaxoFacets 20.91 (12.5%) 20.77 (15.7%) -0.6% ( -25% - 31%) 0.886
AndHighHigh 40.18 (4.6%) 40.06 (4.2%) -0.3% ( -8% - 8%) 0.823
OrNotHighMed 923.89 (3.3%) 921.97 (2.8%) -0.2% ( -6% - 6%) 0.830
HighSloppyPhrase 17.59 (6.1%) 17.55 (4.3%) -0.2% ( -10% - 10%) 0.906
AndHighMed 137.42 (3.7%) 137.17 (3.5%) -0.2% ( -7% - 7%) 0.870
PKLookup 168.11 (2.6%) 168.01 (3.4%) -0.1% ( -5% - 6%) 0.951
BrowseRandomLabelSSDVFacets 10.14 (3.6%) 10.14 (3.3%) 0.0% ( -6% - 7%) 0.989
IntNRQ 204.47 (0.9%) 204.53 (0.6%) 0.0% ( -1% - 1%) 0.907
OrNotHighHigh 1480.42 (3.4%) 1480.99 (2.4%) 0.0% ( -5% - 6%) 0.967
Prefix3 221.07 (15.0%) 221.20 (13.6%) 0.1% ( -24% - 33%) 0.989
Fuzzy1 79.60 (1.8%) 79.74 (1.4%) 0.2% ( -2% - 3%) 0.727
Fuzzy2 75.46 (1.9%) 75.62 (1.6%) 0.2% ( -3% - 3%) 0.697
MedTerm 1563.09 (5.2%) 1567.15 (3.5%) 0.3% ( -8% - 9%) 0.853
MedSpanNear 27.45 (1.7%) 27.54 (1.9%) 0.3% ( -3% - 3%) 0.579
LowPhrase 123.19 (2.2%) 123.66 (1.7%) 0.4% ( -3% - 4%) 0.539
Respell 46.77 (1.8%) 46.97 (1.4%) 0.4% ( -2% - 3%) 0.422
MedPhrase 476.52 (2.1%) 478.54 (1.9%) 0.4% ( -3% - 4%) 0.497
HighPhrase 222.77 (2.3%) 223.82 (1.7%) 0.5% ( -3% - 4%) 0.458
AndHighLow 1161.41 (2.3%) 1167.56 (2.4%) 0.5% ( -4% - 5%) 0.482
OrHighMed 128.58 (3.7%) 129.28 (4.3%) 0.5% ( -7% - 8%) 0.671
LowSpanNear 13.26 (2.3%) 13.34 (2.2%) 0.6% ( -3% - 5%) 0.421
HighSpanNear 10.68 (2.2%) 10.74 (2.1%) 0.6% ( -3% - 5%) 0.392
OrNotHighLow 1314.95 (3.3%) 1324.37 (2.5%) 0.7% ( -4% - 6%) 0.443
OrHighHigh 19.95 (3.9%) 20.11 (4.6%) 0.8% ( -7% - 9%) 0.552
LowIntervalsOrdered 57.37 (4.6%) 57.85 (2.1%) 0.8% ( -5% - 7%) 0.457
AndHighHighDayTaxoFacets 19.87 (2.5%) 20.06 (3.0%) 1.0% ( -4% - 6%) 0.274
MedIntervalsOrdered 56.23 (4.1%) 56.78 (2.3%) 1.0% ( -5% - 7%) 0.354
AndHighMedDayTaxoFacets 81.95 (2.8%) 82.87 (2.0%) 1.1% ( -3% - 5%) 0.138
HighIntervalsOrdered 22.29 (5.1%) 22.56 (3.5%) 1.2% ( -7% - 10%) 0.380
MedTermDayTaxoFacets 39.54 (4.8%) 40.04 (4.9%) 1.3% ( -8% - 11%) 0.410
OrHighMedDayTaxoFacets 4.09 (4.7%) 4.15 (4.2%) 1.5% ( -7% - 10%) 0.304
HighTermTitleBDVSort 42.14 (15.8%) 43.15 (20.0%) 2.4% ( -28% - 45%) 0.675
BrowseMonthSSDVFacets 15.74 (11.0%) 16.17 (13.1%) 2.7% ( -19% - 30%) 0.477
```
--
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-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org