You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "jbellis (via GitHub)" <gi...@apache.org> on 2023/05/05 15:22:24 UTC
[GitHub] [lucene] jbellis opened a new pull request, #12268: add BitSet.clear()
jbellis opened a new pull request, #12268:
URL: https://github.com/apache/lucene/pull/12268
Depending on the implementation, this may be significantly faster than clear(0, length).
In particular, SparseFixedBitSet can use a single Arrays.fill call instead of looping through the backing array.
--
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] jbellis commented on a diff in pull request #12268: add BitSet.clear()
Posted by "jbellis (via GitHub)" <gi...@apache.org>.
jbellis commented on code in PR #12268:
URL: https://github.com/apache/lucene/pull/12268#discussion_r1187475706
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -43,6 +43,16 @@ public static BitSet of(DocIdSetIterator it, int maxDoc) throws IOException {
return set;
}
+ /**
+ * Clear all the bits of the set.
+ *
+ * <p>Depending on the implementation, this may be significantly faster than clear(0, length).
+ */
+ public void clear() {
+ // default implementation for compatibility with possible out-of-tree code
Review Comment:
The patch updates internal methods to use clear() where appropriate, but I'm happy to change the comment.
--
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] jbellis commented on a diff in pull request #12268: add BitSet.clear()
Posted by "jbellis (via GitHub)" <gi...@apache.org>.
jbellis commented on code in PR #12268:
URL: https://github.com/apache/lucene/pull/12268#discussion_r1186274353
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -43,6 +43,13 @@ public static BitSet of(DocIdSetIterator it, int maxDoc) throws IOException {
return set;
}
+ /**
+ * Clear all the bits of the set.
+ *
+ * <p>Depending on the implementation, this may be significantly faster than clear(0, length).
+ */
+ public abstract void clear();
Review Comment:
thanks, that's a good idea
--
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] msokolov commented on a diff in pull request #12268: add BitSet.clear()
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on code in PR #12268:
URL: https://github.com/apache/lucene/pull/12268#discussion_r1187470656
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -43,6 +43,16 @@ public static BitSet of(DocIdSetIterator it, int maxDoc) throws IOException {
return set;
}
+ /**
+ * Clear all the bits of the set.
+ *
+ * <p>Depending on the implementation, this may be significantly faster than clear(0, length).
+ */
+ public void clear() {
+ // default implementation for compatibility with possible out-of-tree code
Review Comment:
"out-of-tree" is confusing me here - do you mean non-Lucene code? I don't know that we need to distinguish since this method could also be called by Lucene internal methods?
--
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] uschindler commented on pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12268:
URL: https://github.com/apache/lucene/pull/12268#issuecomment-1564636408
Thanks. Was also backported to 9.x and will be released with 9.7.
--
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] uschindler commented on pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12268:
URL: https://github.com/apache/lucene/pull/12268#issuecomment-1564523245
I will then press the merge button and cherry-pick it in 9.x branch for next release 9.7.
--
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] uschindler commented on a diff in pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12268:
URL: https://github.com/apache/lucene/pull/12268#discussion_r1186249152
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -43,6 +43,13 @@ public static BitSet of(DocIdSetIterator it, int maxDoc) throws IOException {
return set;
}
+ /**
+ * Clear all the bits of the set.
+ *
+ * <p>Depending on the implementation, this may be significantly faster than clear(0, length).
+ */
+ public abstract void clear();
Review Comment:
For backwards compatibility with possible existing impleentations, this should call by default call `clear(0, length())` instead of being abstract.
--
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] uschindler commented on a diff in pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #12268:
URL: https://github.com/apache/lucene/pull/12268#discussion_r1187617649
##########
lucene/core/src/java/org/apache/lucene/util/BitSet.java:
##########
@@ -43,6 +43,16 @@ public static BitSet of(DocIdSetIterator it, int maxDoc) throws IOException {
return set;
}
+ /**
+ * Clear all the bits of the set.
+ *
+ * <p>Depending on the implementation, this may be significantly faster than clear(0, length).
+ */
+ public void clear() {
+ // default implementation for compatibility with possible out-of-tree code
Review Comment:
Out of tree means code (e.g., outside Lucene) which subclasses abstract BitSet. But as `clear()` without parameters is new it therefore does not implement it. In interfaces this is what "default" classifier on method means.
The comment was correct but misleading. Backwards compatibility is here about code subclassing a public class with well defined interface. Those can't suddenly add abstract methods without a default implementation.
--
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] jbellis commented on pull request #12268: add BitSet.clear()
Posted by "jbellis (via GitHub)" <gi...@apache.org>.
jbellis commented on PR #12268:
URL: https://github.com/apache/lucene/pull/12268#issuecomment-1564588827
done!
--
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] uschindler merged pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #12268:
URL: https://github.com/apache/lucene/pull/12268
--
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] uschindler commented on pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12268:
URL: https://github.com/apache/lucene/pull/12268#issuecomment-1564522169
Hi,
sorry this went out of my view. Could you please add a CHANGES.txt entry in the 9.7 part of the file?
--
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] uschindler commented on pull request #12268: add BitSet.clear()
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #12268:
URL: https://github.com/apache/lucene/pull/12268#issuecomment-1536490875
To me this looks like a good idea. What do other think?
Pro:
- Cleaner code when you want to reuse an already existing BitSet instance
- Faster in some cases
Cons:
- Additional method to be implemented (no longer needed as a default impl exists)
- We could also just modify ` SparseFixedBitset#clear(start, end)` to check start/end and use Arrays.fill() if `start==0 && end==length`
--
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