You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/11/06 10:10:56 UTC
[GitHub] [commons-collections] Claudenw opened a new pull request, #357: Added Hasher.isEmpty() and appropriate tests
Claudenw opened a new pull request, #357:
URL: https://github.com/apache/commons-collections/pull/357
Adds isEmpty() method to Hasher and implements appropriate tests.
Fixes COLLECTIONS-830
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] aherbert commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1015814250
##########
src/main/java/org/apache/commons/collections4/bloomfilter/Hasher.java:
##########
@@ -61,4 +61,15 @@ default IndexProducer uniqueIndices(Shape shape) {
return Hasher.this.indices(shape).forEachIndex(IndexFilter.create(shape, consumer));
};
}
+
+ /**
+ * Returns {@code true} if the hasher is empty and will return no values.
+ *
+ * By default this method returns {@code false}.
Review Comment:
Needs a `<p>` tag. Change to `<p>The default implementation returns ... </p>`
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw closed pull request #357: COLLECTIONS-830: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw closed pull request #357: COLLECTIONS-830: Added Hasher.isEmpty() and appropriate tests
URL: https://github.com/apache/commons-collections/pull/357
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] codecov-commenter commented on pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1304765508
# [Codecov](https://codecov.io/gh/apache/commons-collections/pull/357?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#357](https://codecov.io/gh/apache/commons-collections/pull/357?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9eeb3dd) into [master](https://codecov.io/gh/apache/commons-collections/commit/3071aea62de6caa0416e873b3930d08f3c703cc7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (3071aea) will **increase** coverage by `0.05%`.
> The diff coverage is `83.33%`.
```diff
@@ Coverage Diff @@
## master #357 +/- ##
============================================
+ Coverage 81.21% 81.27% +0.05%
- Complexity 4599 4604 +5
============================================
Files 289 289
Lines 13445 13451 +6
Branches 1973 1974 +1
============================================
+ Hits 10920 10932 +12
+ Misses 1924 1921 -3
+ Partials 601 598 -3
```
| [Impacted Files](https://codecov.io/gh/apache/commons-collections/pull/357?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...collections4/bloomfilter/EnhancedDoubleHasher.java](https://codecov.io/gh/apache/commons-collections/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L2Jsb29tZmlsdGVyL0VuaGFuY2VkRG91YmxlSGFzaGVyLmphdmE=) | `98.24% <0.00%> (-1.76%)` | :arrow_down: |
| [...pache/commons/collections4/bloomfilter/Hasher.java](https://codecov.io/gh/apache/commons-collections/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L2Jsb29tZmlsdGVyL0hhc2hlci5qYXZh) | `100.00% <ø> (ø)` | |
| [...ons/collections4/bloomfilter/HasherCollection.java](https://codecov.io/gh/apache/commons-collections/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L2Jsb29tZmlsdGVyL0hhc2hlckNvbGxlY3Rpb24uamF2YQ==) | `100.00% <100.00%> (ø)` | |
| [...commons/collections4/map/AbstractReferenceMap.java](https://codecov.io/gh/apache/commons-collections/pull/357/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvY29sbGVjdGlvbnM0L21hcC9BYnN0cmFjdFJlZmVyZW5jZU1hcC5qYXZh) | `80.74% <0.00%> (+2.59%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] aherbert commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
aherbert commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1015160690
##########
src/main/java/org/apache/commons/collections4/bloomfilter/Hasher.java:
##########
@@ -61,4 +61,10 @@ default IndexProducer uniqueIndices(Shape shape) {
return Hasher.this.indices(shape).forEachIndex(IndexFilter.create(shape, consumer));
};
}
+
+ /**
+ * Returns {@code true} if the hasher is empty and will return no values.
+ * @return {@code true} if the hasher is empty and will return no values.
+ */
+ boolean isEmpty();
Review Comment:
I think this should have a default implementation of `return false;`.
There are very few places where it has to be overridden. When this has a default implementation the interface can remain a functional interface.
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractHasherTest.java:
##########
@@ -85,4 +86,7 @@ public void testHashing(int k, int m) {
});
assertEquals(1, count[0], "did not exit early");
}
+
+ @Test
+ public abstract void testIsEmpty();
Review Comment:
Why is this abstract?
```Java
public void testIsEmpty() {
Assertions.assertTrue(createEmptyHasher().isEmpty());
Assertions.assertFalse(createHasher().isEmpty());
}
```
The specific use case in HasherCollection with a non-empty collection of empty hashers can be left to override this test.
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
I am fine with a non-stream method to avoid the stream object creation overhead.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] aherbert commented on pull request #357: COLLECTIONS-830: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1309993863
Removal of HasherCollection would eliminate all the possible confusion around its use given that it has behaviour that can be, at least in part, either: no items; a single item (Hasher); collection of items (List<Hasher>); or a bitwise combination of items (BloomFilter).
I suggest the functionality can be matured outside of the core functionality in Collections and then integrated with all the appropriate code for the use case(s).
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1304803054
Hm, `HasherCollection` is not a `Collection`, instead it is a `List` wrapper, so I suggest it be renamed `HasherList` and that it implements `Iterable<Hasher>`. WDYT? It seems to heavy to implement `List` itself.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] kinow commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1014810859
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
Bit rusty in Java & streams, but I **think** this is equivalent to:
```java
return hashers.stream().noneMatch(Hasher::isEmpty);
```
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1015756682
##########
src/test/java/org/apache/commons/collections4/bloomfilter/AbstractHasherTest.java:
##########
@@ -85,4 +86,7 @@ public void testHashing(int k, int m) {
});
assertEquals(1, count[0], "did not exit early");
}
+
+ @Test
+ public abstract void testIsEmpty();
Review Comment:
changed
##########
src/main/java/org/apache/commons/collections4/bloomfilter/Hasher.java:
##########
@@ -61,4 +61,10 @@ default IndexProducer uniqueIndices(Shape shape) {
return Hasher.this.indices(shape).forEachIndex(IndexFilter.create(shape, consumer));
};
}
+
+ /**
+ * Returns {@code true} if the hasher is empty and will return no values.
+ * @return {@code true} if the hasher is empty and will return no values.
+ */
+ boolean isEmpty();
Review Comment:
changed
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on pull request #357: COLLECTIONS-830: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1309958426
Perhaps the HasherCollection does not belong in the library. It may make more sense to withdraw this pull request, close the COLLECTIONS-830 as won't fix and create another pull request to remove the HasherCollection. I know there were other issues that @aherbert had with that implementation anyway.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1015757510
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
Leaving as non-stream method to avoid the stream object creation overhead.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1307730779
> I am curious as to why you would have an empty hasher. Is this not a programming error? Basically it means you are testing if `nothing` is present in a filter, or adding `nothing` to a filter.
First I need to define a "reference" Bloom filter. Consider the properties of some object. If you can index and search a collection of Bloom filters then you can take the properties of an object, create a Bloom filter from the values, and use that as a reference to the object. Now you can search the collection of filters by creating a Bloom filter that contains only those values that you want to locate. This usage of Bloom filters is really only available once the indexed collection is possible. The indexed collection is called a multidimensional Bloom filter -- actually any collection of Bloom filters is a multidimensional Bloom filter.
In the case of a searching multidimensional Bloom filter in a client-server environment where:
- the client and the server agree on the underlying hashing algorithm and the use of the EnhancedDoubleHasher.
- the Bloom filter being generated is not a standard "gateway" Bloom filter but a "reference" filter.
- The client creates the hashers for the filter and places them into a collection which it then passes to the server.
- The server deserializes the hashers into a HasherCollection.
- If the hasher is empty the server can return the entire collection or can return an error.
- otherwise the server performs the search building the internal Bloom filters as required by the multidimensional Bloom filter algorithm.
Notice that the empty hasher allows a short circuit of the expensive search.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] kinow commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1014810732
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
+ }
Review Comment:
Bit rusty with Java & streams, but I **think** this is equivalent to:
```java
return hashers.stream().noneMatch(Hasher::isEmpty);
```
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
garydgregory commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1014829652
##########
src/test/java/org/apache/commons/collections4/bloomfilter/EnhancedDoubleHasherTest.java:
##########
@@ -52,6 +53,11 @@ protected int getHasherSize(Hasher hasher) {
return 1;
}
+ @Override
+ public void testIsEmpty() {
+ assertFalse( createHasher().isEmpty() );
Review Comment:
No extra spaces around parens.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1015757174
##########
src/test/java/org/apache/commons/collections4/bloomfilter/EnhancedDoubleHasherTest.java:
##########
@@ -52,6 +53,11 @@ protected int getHasherSize(Hasher hasher) {
return 1;
}
+ @Override
+ public void testIsEmpty() {
+ assertFalse( createHasher().isEmpty() );
Review Comment:
fixed
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] kinow commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1014811662
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
Out of the loop, so can't comment on the addition of that method, but the rest of the code looks Ok :+1:
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] kinow commented on a diff in pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #357:
URL: https://github.com/apache/commons-collections/pull/357#discussion_r1014810859
##########
src/main/java/org/apache/commons/collections4/bloomfilter/HasherCollection.java:
##########
@@ -66,6 +66,16 @@ public HasherCollection(Hasher... hashers) {
this(Arrays.asList(hashers));
}
+ @Override
+ public boolean isEmpty() {
+ for (Hasher h : hashers) {
+ if (!h.isEmpty()) {
+ return false;
+ }
+ }
+ return true;
Review Comment:
Bit rusty with Java & streams, but I **think** this is equivalent to:
```java
return hashers.stream().noneMatch(Hasher::isEmpty);
```
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] aherbert commented on pull request #357: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
aherbert commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1307849658
IIUC the use of `isEmpty` is to prevent a client sending an empty something to a server which would waste its time, or use it as a signal. The former seems like an error by the client (or a denial of service attack); the later can be achieved by other means, for example sending empty/signalling bytes before deserialisation on the server side. This would avoid having to create the (empty) HasherCollection altogether.
It seems like quite an edge case scenario. If you have to use an empty Hasher as a signal then it can be done on the server side using:
```Java
static boolean isEmpty(Hasher h) {
boolean[] flag = {true};
// Shape can be precomputed for this purpose
h.indices(Shape.fromKM(1, 1)).forEachIndex(i -> flag[0] = false);
return flag[0];
}
```
If you have to deserialise the input bytes to a Hasher then just do it as a customisation of your choice of Hasher with an `isEmpty` method. If you are using an EnhancedDoubleHasher then note that this cannot be "empty" even if the initial and increment are zero given that Shape numberOfHash functions is always `> 0` so at least 1 index will be output.
In general any Hasher should output indices. If you created a hasher that contains nothing, this itself is something that should generate output indices. The Hasher simply outputs indices based on a source, and ideally the output sequence is fairly unique among possible input sources.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-collections] Claudenw commented on pull request #357: COLLECTIONS-830: Added Hasher.isEmpty() and appropriate tests
Posted by GitBox <gi...@apache.org>.
Claudenw commented on PR #357:
URL: https://github.com/apache/commons-collections/pull/357#issuecomment-1311613070
HassherCollection to be removed from code base
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org