-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[COLLECTIONS-795] Add a new Iterator to allowing zipping over two iterators of different types #238
Conversation
* @param <L> the left elements' type | ||
* @param <R> the right elements' type | ||
*/ | ||
public class ZippedTupleIterator<L, R> implements Iterator<ZippedTuple<L, R>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this prefixed with "zip" when it has nothing to do with zip files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is related to zipper. Like Python stdlib's zip function. Scala and Clojure also have a similar function I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe but the prefix here is "zipped" not "zipper", terrible name IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not good naming things, so can't really suggest a better one. The code example in the Python docs uses zipped
too for the variable that contains the tuples returned with zip
(that were zipped), so that should sound OK for users coming from Python (or JS using Lodash's zip/unzip which I'm using in my current project). But if that's confusing for other users, we can try to think in a better name I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use PairedIterator
and PairedIterable
names. Hope is acceptable to all.
src/test/java/org/apache/commons/collections4/iterators/ZippedTupleIteratorTest.java
Outdated
Show resolved
Hide resolved
The CI flagged failing tests are not stemming from any code that I have touched, |
checkstyles fails.
https://travis-ci.com/github/apache/commons-collections/jobs/508633787
[ERROR] src/main/java/org/apache/commons/collections4/IterableUtils.java:[574,21] (whitespace) WhitespaceAfter: ',' is not followed by whitespace. |
Click on the link of the failed Travis CI builds. Then choose one of the JDK versions (e.g. 8). There you should find the logs of the job that failed. For instance:
|
thanks @XenoAmess and @kinow The sytle checks & corrections can be simplified using spotless plugin. |
Added tests to improve coverage |
What are the next steps? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to take a look again later this week but did I miss something or are there no tests for empty lists, lists of unequal sizes, null inputs?
src/test/java/org/apache/commons/collections4/iterators/PairedIteratorTest.java
Outdated
Show resolved
Hide resolved
* | ||
* @param <L> the left elements' type | ||
* @param <R> the right elements' type | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to think about this because Common Lang already defines different kinds of Pair classes...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually use ImmutablePair
, but that is in apache/commons-lang3 , that is not a dependency in this project and didn't think should add, but open to ideas. If you point me to a Pair Class, would be open to using an existing pair class.
src/test/java/org/apache/commons/collections4/PairedIterableTest.java
Outdated
Show resolved
Hide resolved
private ArrayList<Integer> largeIntsList = null; | ||
|
||
// Unequal sized lists | ||
private static final int SMALL_LIST_SIZE = 20; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you expect the sizes to be different, you should assert that fact somewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added two tests: testLeftIteratorLargerThanRight
and testRightIteratorLargerThanLeft
to match the tests of same name from PairedIteratorTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi All,
See 2 minor comments. Another class to inspect is the existing ZippingIterator
which should be considered for reuse; but note its different 'switching' aspect. Also, ZippingIterator
will work for any number of input iterators (ZippingIterator.ZippingIterator(Iterator<? extends E>...)
), not just two. So perhaps this PR should as well, which would remove the need for the pair class.
src/main/java/org/apache/commons/collections4/IteratorUtils.java
Outdated
Show resolved
Hide resolved
@garydgregory there is one big difference with ZippingIterator, |
@garydgregory gentle bump up. |
Gentle bump: any comments or changes today I should be doing? |
Gentle Bump: Any review comments or improvements |
No bumping needed. We are all volunteers. We are all busy. Please be patient. |
sorry to nag! Any updates? |
@anantdamle |
I'll try to take a look this week... |
Any updates on this one? |
…llections (apache#301) * [COLLECTIONS-811] Integrate Guava Testlib tests for Apache Commons Collections * [COLLECTIONS-811] Add tests for Lists too, thanks to @ben-manes
Bumps [maven-pmd-plugin](https://github.com/apache/maven-pmd-plugin) from 3.16.0 to 3.17.0. - [Release notes](https://github.com/apache/maven-pmd-plugin/releases) - [Commits](apache/maven-pmd-plugin@maven-pmd-plugin-3.16.0...maven-pmd-plugin-3.17.0) --- updated-dependencies: - dependency-name: org.apache.maven.plugins:maven-pmd-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]>
* Fixed some unit tests * First set with complete test cases. * Cleaned up hasher collecton processing * cleaned up code * added license headers * Refactored and cleaned up Moved to dependency on BitMapProducer, IndexProducer and BitCountProducer to retrieve internal representations of the data. * Added license header. * Updated documentation * Fixed bug and added tests * Added "@SInCE 4.5" where necessary * Added BitMapProducer constructor to SimpleBloomFilter * added BitMapProducer.fromLongArray() and Hasher.isEmpty() * Changes to speed up Simple filter processing * Null hasher used when a hasher is required but no values are available. * Added Hasher.Filter and Hasher.FilteredIntConsumer * Updated documentation + formatted. * Added license * fixed checkstyle issues * fixed javadoc issues * fixed test issue * fixed javadoc issues * Reduced the acceptable delta for p tests * Updated docs and test cases * Updated docs and test cases * fixed issue with Shape javadoc * Added more test coverage. * fixed formatting issues * Updated tests to use assertThrows. * fixed indents * Added constructor with IndexProducer * Fixed issue with compare and different length bitMap arrays * fixed formatting issues * Efficiency changes cleaned up asIndexArray BitMapProducer to IndexProducer conversion * changed XProviers to use XPredicates * Removed NoMatchException * Removed unneeded BitMap funcs Moves isSparse() to Shape. * fixed checkstyle issues * Fixed javadoc errors * simplified parameter in BitMapProducer.fromIndexProducer * fixed tests * added BitMapping verification * Added more tests * Added more tests * Fixed typos * Changes requested by aherbert * fixed "bit map" in documentation * Renamed tests * Removed blank lines * changed new X<foo> to new X<> * updated documentation * Added BloomFilter.copy() * changed ArrayCountingBloomFilter to use copy() method * cleaned up numberOfBitsMaps() * added asBitMapArray() and makePredicate() to BitMapProducer * Moved asIndexArray() to IndexProducer * harmonized Simple and Sparse Bloom filter constructors * Implemented AbstractCountingBloomFilter.asindexArray() * updated documentation * fixed up NullHasher * implemented hasher filter * Fixed style issues * added default SimpleHasher increment. * Added modulus calculation to SimpleHasher * fixed Hashing issues * moved hasher/filter/* to /hasher * moved bloomfilter/hasher to bloomfilter * fixed up checkstyle issues * Made Filter -> IndexFilter -w- factory * moved IndexFilter into Hasher * updated hashing tests & fixed checksyle * removed SingleItemhasherCollection as associated methods * Fixed some unit tests * First set with complete test cases. * Cleaned up hasher collecton processing * cleaned up code * added license headers * Refactored and cleaned up Moved to dependency on BitMapProducer, IndexProducer and BitCountProducer to retrieve internal representations of the data. * Added license header. * Updated documentation * Fixed bug and added tests * Added "@SInCE 4.5" where necessary * Added BitMapProducer constructor to SimpleBloomFilter * added BitMapProducer.fromLongArray() and Hasher.isEmpty() * Changes to speed up Simple filter processing * Null hasher used when a hasher is required but no values are available. * Added Hasher.Filter and Hasher.FilteredIntConsumer * Updated documentation + formatted. * Added license * fixed checkstyle issues * fixed javadoc issues * fixed test issue * fixed javadoc issues * Reduced the acceptable delta for p tests * Updated docs and test cases * Updated docs and test cases * fixed issue with Shape javadoc * Added more test coverage. * fixed formatting issues * Updated tests to use assertThrows. * fixed indents * Added constructor with IndexProducer * Fixed issue with compare and different length bitMap arrays * fixed formatting issues * Efficiency changes cleaned up asIndexArray BitMapProducer to IndexProducer conversion * changed XProviers to use XPredicates * Removed NoMatchException * Removed unneeded BitMap funcs Moves isSparse() to Shape. * fixed checkstyle issues * Fixed javadoc errors * simplified parameter in BitMapProducer.fromIndexProducer * fixed tests * added BitMapping verification * Added more tests * Added more tests * Fixed typos * Changes requested by aherbert * fixed "bit map" in documentation * Renamed tests * Removed blank lines * changed new X<foo> to new X<> * updated documentation * Added BloomFilter.copy() * changed ArrayCountingBloomFilter to use copy() method * cleaned up numberOfBitsMaps() * added asBitMapArray() and makePredicate() to BitMapProducer * Moved asIndexArray() to IndexProducer * harmonized Simple and Sparse Bloom filter constructors * Implemented AbstractCountingBloomFilter.asindexArray() * updated documentation * fixed up NullHasher * implemented hasher filter * Fixed style issues * added default SimpleHasher increment. * Added modulus calculation to SimpleHasher * fixed Hashing issues * moved hasher/filter/* to /hasher * moved bloomfilter/hasher to bloomfilter * fixed up checkstyle issues * Made Filter -> IndexFilter -w- factory * moved IndexFilter into Hasher * updated hashing tests & fixed checksyle * removed SingleItemhasherCollection as associated methods * fixed javadoc issues * fixed javadoc issues * added checks for BitMapProducer limits and index limits * updated tests * added tests * fixed checkstyle issues * fixed formatting and test coverage * fixed javadoc issue * put back checkstyle.xml * switched to forEachBitMapPair * updated BitMap and Index array production * fixed merge with BitMapProducer * Cleaned up formatting * fixed checkstyle issues * fixed coding issues * updated documentation * simplified test * removed unwanted merge files * removed duplicate entry * put back test that incorrectly removed * fixed asIndexArray error * fixed checkstyle errors * Changes for last review
Bumps [ossf/scorecard-action](https://github.com/ossf/scorecard-action) from 2.1.3 to 2.2.0. - [Release notes](https://github.com/ossf/scorecard-action/releases) - [Changelog](https://github.com/ossf/scorecard-action/blob/main/RELEASE.md) - [Commits](ossf/scorecard-action@80e868c...08b4669) --- updated-dependencies: - dependency-name: ossf/scorecard-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [guava-testlib](https://github.com/google/guava) from 32.0.1-jre to 32.1.0-jre. - [Release notes](https://github.com/google/guava/releases) - [Commits](https://github.com/google/guava/commits) --- updated-dependencies: - dependency-name: com.google.guava:guava-testlib dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [guava-testlib](https://github.com/google/guava) from 32.1.0-jre to 32.1.1-jre. - [Release notes](https://github.com/google/guava/releases) - [Commits](https://github.com/google/guava/commits) --- updated-dependencies: - dependency-name: com.google.guava:guava-testlib dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
OrderedPropertiesFactory
Passes locally, fails on GHA
* Make AbstractPatriciaTrie public * Added javadocs
Bumps [actions/setup-java](https://github.com/actions/setup-java) from 3.11.0 to 3.12.0. - [Release notes](https://github.com/actions/setup-java/releases) - [Commits](actions/setup-java@v3.11.0...v3.12.0) --- updated-dependencies: - dependency-name: actions/setup-java dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…dicate) (apache#409) resolves grammar and punctuation issues
…ache#411) Bumps [com.google.guava:guava-testlib](https://github.com/google/guava) from 32.1.1-jre to 32.1.2-jre. - [Release notes](https://github.com/google/guava/releases) - [Commits](https://github.com/google/guava/commits) --- updated-dependencies: - dependency-name: com.google.guava:guava-testlib dependency-type: direct:development update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
apache#404, apache#405, apache#441. Bump com.google.guava:guava-testlib from 32.1.1-jre to 32.1.2-jre apache#411
…s_795 # Conflicts: # src/test/java/org/apache/commons/collections4/iterators/PairedIteratorTest.java
Closing this as rebase to master created 100s of commits polluting the PR. |
[COLLECTIONS-795] Add a new Iterator to allowing zipping over two iterators of different types.