Skip to content

remove deprecated old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering#451

Merged
tyrasd merged 11 commits into
masterfrom
issue-324
May 31, 2022
Merged

remove deprecated old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering#451
tyrasd merged 11 commits into
masterfrom
issue-324

Conversation

@tyrasd
Copy link
Copy Markdown
Member

@tyrasd tyrasd commented Apr 21, 2022

addresses #324:

  • removes old osmType filter methods
  • removes old osmTag filter methods
  • gets rid of (osmTag(key, valueRegexp)) method. reason: it was rarely used and never really worked when using an ignite backend. Also, it can be implemented relatively easily manually if needed in an analysis.
  • implements groupById and aggregateByGeometry also after a filter, map or flatMap function was specified on a mapReducer. (solving the issue outlined in Removing deprecate MapReducerSettings #442 (comment))
    the following corner case still causes an UnsupportedOperationException: mapRed.filter("…").aggregateBy(…).aggregateByGeometry(…) but it can be worked around relatively easily by either swapping the two aggregateBy's (mapRed.filter("…").aggregateByGeometry(…).aggregateBy(…)) or by delaying the filtering (mapRed.aggregateBy(…).aggregateByGeometry(…).filter("…")).

Corresponding issue

Closes #324

Checklist

@tyrasd tyrasd added enhancement New feature or request user experience Enhances the usability of OSHDB labels Apr 21, 2022
@tyrasd tyrasd changed the title deprecate old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering 🚧 deprecate old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering Apr 21, 2022
@tyrasd
Copy link
Copy Markdown
Member Author

tyrasd commented Apr 21, 2022

I'm not sure why this seemingly breaks the following two tests on Jenkins:

TestMapReduceOSHDBH2Singlethread.testTimeoutMapReduce Expected exception: org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException
TestMapReduceOSHDBH2Singlethread.testTimeoutStream Expected exception:org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException

These two tests work fine locally here. Will need to investigate 🤔

@rtroilo
Copy link
Copy Markdown
Member

rtroilo commented Apr 22, 2022

I'm not sure why this seemingly breaks the following two tests on Jenkins:

TestMapReduceOSHDBH2Singlethread.testTimeoutMapReduce Expected exception: org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException
TestMapReduceOSHDBH2Singlethread.testTimeoutStream Expected exception:org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException

These two tests work fine locally here. Will need to investigate thinking

Don't worry about this tests, we fixed them already in #450

@joker234 joker234 force-pushed the issue-324 branch 2 times, most recently from 0150186 to 4704607 Compare April 22, 2022 16:56
@joker234
Copy link
Copy Markdown
Member

I'm not sure why this seemingly breaks the following two tests on Jenkins:

TestMapReduceOSHDBH2Singlethread.testTimeoutMapReduce Expected exception: org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException
TestMapReduceOSHDBH2Singlethread.testTimeoutStream Expected exception:org.heigit.ohsome.oshdb.util.exceptions.OSHDBTimeoutException

These two tests work fine locally here. Will need to investigate thinking

Even though we fixed a lot in #450, the tests were still failing. I suspect that .filter(…) is much faster than .osmEntityFilter(…). The tests worked (locally) sometimes for me with a lower timeout setting (like 5 or 10 ms), sometimes not. With setting the timeouts to 1 ms it worked all the time (see 4704607).

@tyrasd tyrasd changed the title 🚧 deprecate old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering 🚧 remove deprecated old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering May 4, 2022
@tyrasd tyrasd changed the title 🚧 remove deprecated old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering remove deprecated old type+tag filters and allow groupById and aggregatebyGeometry after mapping/filtering May 5, 2022
@tyrasd tyrasd requested a review from joker234 May 5, 2022 14:23
@joker234 joker234 added this to the release 1.0 milestone May 5, 2022
@joker234 joker234 requested a review from rtroilo May 5, 2022 17:18
@joker234 joker234 added the waiting for review This pull request needs a code review label May 6, 2022
Comment thread oshdb-api/src/main/java/org/heigit/ohsome/oshdb/api/mapreducer/MapReducer.java Outdated
Comment on lines +145 to +143
.aggregateByTimestamp()
.aggregateByGeometry(getSubRegions())
.aggregateByTimestamp(OSMEntitySnapshot::getTimestamp)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What was the reason to change the order of the aggregation from timestamp, geometry to geometry, timestamp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

fixed in 40dba31 by simplifying the implementation of the "automatic" aggregation methods. As as sideeffect, now all(?) aggregation combinations are allowed and possible regardless of their order, for example: mapRed.aggregateByGeometry(…).aggregateByTimestamp() or mapRed.map(…).aggregateByTimestamp().aggregateByGeometry(…)

joker234 and others added 3 commits May 12, 2022 15:08
by adding access to the original "root" object of the map-reducer's view to the MapFunction class.
Copy link
Copy Markdown
Member

@rtroilo rtroilo left a comment

Choose a reason for hiding this comment

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

just a quick comment, why the widening to Object, see suggestions, is necessary. It seems for me that the tests run successfully through even without this.

Comment thread oshdb-api/src/main/java/org/heigit/ohsome/oshdb/api/mapreducer/MapReducer.java Outdated
Comment thread oshdb-api/src/main/java/org/heigit/ohsome/oshdb/api/mapreducer/MapReducer.java Outdated
Co-authored-by: Rafael Troilo <rafael.troilo@heigit.org>
@tyrasd
Copy link
Copy Markdown
Member Author

tyrasd commented May 31, 2022

just a quick comment, why the widening to Object, see suggestions, is necessary. It seems for me that the tests run successfully through even without this.

Good catch, you're right. It was probably an oversight / remainder of some intermediate change I made that wasn't necessary in the end. I've now restored the specific types in the cast.

@tyrasd tyrasd requested a review from rtroilo May 31, 2022 08:32
rtroilo
rtroilo previously approved these changes May 31, 2022
Copy link
Copy Markdown
Member

@rtroilo rtroilo left a comment

Choose a reason for hiding this comment

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

Thank you .

@tyrasd tyrasd removed the waiting for review This pull request needs a code review label May 31, 2022
@tyrasd tyrasd removed the request for review from joker234 May 31, 2022 12:09
@tyrasd tyrasd merged commit 62d06e0 into master May 31, 2022
@tyrasd tyrasd deleted the issue-324 branch May 31, 2022 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request user experience Enhances the usability of OSHDB

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[oshdb-api] drop deprecated mapreducer methods

3 participants