Skip to content

De-duplicate stops returned by stopsByRadius#5366

Merged
leonardehrenfried merged 2 commits into
opentripplanner:dev-2.xfrom
ibi-group:remove-duplicate-nearby-stops
Sep 25, 2023
Merged

De-duplicate stops returned by stopsByRadius#5366
leonardehrenfried merged 2 commits into
opentripplanner:dev-2.xfrom
ibi-group:remove-duplicate-nearby-stops

Conversation

@leonardehrenfried

Copy link
Copy Markdown
Member

Summary

@binh-dam-ibigroup has reported that there are cases where the stopsByRadius GraphQL query returns duplicate stops. This PR removes the duplicates.

It introduces a new Util class that lets you remove duplicates from a collection even when the thing to deduplicate on is not the whole entity but a field of it.

Unit tests

Added.

Documentation

Javadoc.

@leonardehrenfried leonardehrenfried added !Bug Apply to issues describing a bug and PRs witch fixes it. IBI Developed by or important for IBI Group labels Sep 20, 2023
@leonardehrenfried
leonardehrenfried requested a review from a team as a code owner September 20, 2023 07:50
@leonardehrenfried leonardehrenfried changed the title Deduplicate stops returned by stopsByRadius De-duplicate stops returned by stopsByRadius Sep 20, 2023
@codecov

codecov Bot commented Sep 20, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 80.00% and project coverage change: +0.01% 🎉

Comparison is base (3e36162) 66.48% compared to head (8d39386) 66.49%.
Report is 22 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5366      +/-   ##
=============================================
+ Coverage      66.48%   66.49%   +0.01%     
+ Complexity     15238    15236       -2     
=============================================
  Files           1787     1786       -1     
  Lines          69325    69271      -54     
  Branches        7319     7291      -28     
=============================================
- Hits           46088    46064      -24     
+ Misses         20758    20739      -19     
+ Partials        2479     2468      -11     
Files Changed Coverage Δ
...ext/gtfsgraphqlapi/datafetchers/QueryTypeImpl.java 19.83% <0.00%> (ø)
...opentripplanner/framework/lang/PredicateUtils.java 66.66% <66.66%> (ø)
...pentripplanner/ext/geocoder/StopClusterMapper.java 96.55% <100.00%> (-0.23%) ⬇️
...routing/graphfinder/StopFinderTraverseVisitor.java 100.00% <100.00%> (ø)
...planner/routing/graphfinder/StreetGraphFinder.java 100.00% <100.00%> (ø)

... and 22 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue mentioned is indeed resolved. Thanks!

@vpaturet

Copy link
Copy Markdown
Contributor

Oracle discourages the use of stateful filters (https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/stream/package-summary.html#Statelessness), an alternative implementation could use collectors to achieve the same effect:

return stopsFound.stream().distinct().collect(Collectors.toMap(nearbyStop -> nearbyStop.stop, Function.identity(), (n1, n2) -> n1 )).values().stream().toList();

In this particular case, the impact is limited, so approving anyway.

@leonardehrenfried
leonardehrenfried merged commit d4411fd into opentripplanner:dev-2.x Sep 25, 2023
@leonardehrenfried
leonardehrenfried deleted the remove-duplicate-nearby-stops branch September 25, 2023 07:31
t2gran pushed a commit that referenced this pull request Sep 25, 2023
@leonardehrenfried

Copy link
Copy Markdown
Member Author

That's an excellent point which I will implement in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

!Bug Apply to issues describing a bug and PRs witch fixes it. IBI Developed by or important for IBI Group

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants