Add support for relative filtering #4740

soshial · 2021-09-10T07:43:39Z

This PR tries to address problem #4735.

tananaev · 2021-09-10T18:16:30Z

src/main/java/org/traccar/handler/FilterHandler.java

+                }
+
+                Position previous = Context.getDataManager().getPrevPosition(position.getDeviceId(), newfixTime);
+                Position next = Context.getDataManager().getNextPosition(position.getDeviceId(), newfixTime);


Can you please provide details on why we need "next" position.

The backend has configured MinPeriod filter set to 2 seconds. Imagine that these Positions were received by the server:

Position A: fixTime = 18:18:01

Position C: fixTime = 18:18:05

Then after 10 seconds, server receives position B with fixTime = 18:18:04. As it turns out, it was not delivered immediately because device was offline for 3 seconds, because it was switching between networks.

Time-wise position B is far enough from position A (previous location), but it's too close to position C (next position). Hence, we need to take into account next position as well.

It makes sense in theory, but I'm not sure if it makes sense to actually implement it. You basically make 3 expensive SQL requests for every location report. It will cripple pretty much any server that runs any significant number of devices.

I agree that with thousands of tracking devices this will have deteriorating impact. But for private use it will not be a problem. No doubt, that we should make this option disabled by default.

What do you say, if I introduce this optimization: let's load next and prev positions only in case any of these filters are turned active:

filterDistance

filterMaxSpeed

filterMinPeriod

So if we only duplicate filter is active, than we have only 1 request instead of 3.

I'm mostly worried about value of this feature vs cost of maintaining it going forward.

We can also make lazy loading of last / previous position. What do you say? Only in the case when at least one of the corresponding filters is activated.

Makes sense to me.

Check this out:

line 149: moved skipAttr to the top
line 176: lazy instantiation of previous

Looks fine, but please update PR so I can review it more thoroughly.

OK, I have pushed a new commit.

tananaev

Looks good. Add a few minor comments, mostly about formatting.

src/main/java/org/traccar/handler/FilterHandler.java

src/main/java/org/traccar/config/Keys.java

src/main/java/org/traccar/database/DataManager.java

setup/default.xml

src/main/java/org/traccar/handler/FilterHandler.java

soshial · 2021-09-13T06:58:51Z

It is. I force pushed the commit.

tananaev · 2021-09-13T16:41:39Z

setup/default.xml

@@ -48,6 +48,10 @@
        SELECT * FROM tc_positions WHERE deviceId = :deviceId AND fixTime BETWEEN :from AND :to ORDER BY fixTime
    </entry>

+    <entry key='database.selectPrecedingPosition'>
+        SELECT * FROM tc_positions WHERE deviceId = :deviceId AND fixTime lte :time ORDER BY fixTime DESC LIMIT 1


One last thing. I noticed that you are using lte. Is it a standard SQL syntax that will work in every SQL server? Can you please provide a reference because I couldn't find any information about it.

Honestly, I thought that you were using some kind of library to parse SQL stored in XML. Did you figure out correctly, that you wrote it yourself. I couldn't use < sign since it's forbidden inside XML tag.

We may use this:
SELECT * FROM tc_positions WHERE deviceId = :deviceId AND fixTime <= :time ORDER BY fixTime DESC LIMIT 1
or
SELECT * FROM tc_positions WHERE deviceId = :deviceId AND NOT (fixTime > :time) ORDER BY fixTime DESC LIMIT 1

What do you think?

I think just <= should be fine, so let's go with option 1.

tananaev

Also, just to confirm. Have you tested the latest to make sure everything works?

tananaev · 2021-09-13T20:07:02Z

src/main/java/org/traccar/database/DataManager.java

+                .executeQuery(Position.class);
+        if (positions.isEmpty()) {
+            return null;
+        } else {
+            return positions.iterator().next();
+        }


Sorry, just noticed another thing. Can we use executeQuerySingle here instead?

soshial · 2021-09-18T10:10:42Z

I have tested the code and I found a serious problem (as it seems to me), that have existed in traccar even before my pull request. As it turns out, traccar doesn't save fractional TIMESTAMP values, it saves only seconds (not milliseconds). Which means that duplicate comparison didn't work for protocols that send milliseconds:

position.getFixTime().getTime() == last.getFixTime().getTime() returns false for seemingly duplicate position (that I send repeatedly). When comparing to last received position (as was implemented before) this problem did not appear (because positions were kept inside IdentityManager in RAM). But if we take values from DB that cuts off milliseconds, it becomes a problem.

Compare:

the new received position had value 1626692627800
and the position from DB has value 1626692627000

Honestly, I really think that traccar should save milliseconds in the database. Some usecases demand location data more frequent than once per second.

Something like this: ALTER TABLE tc_positions CHANGE COLUMN fixtime TIMESTAMP(6) and other time-related columns.

tananaev · 2021-09-18T15:02:47Z

What database are you using?

soshial · 2021-09-18T15:03:44Z

MariaDB.

tananaev · 2021-09-18T15:05:12Z

Is there a universal solution?

soshial · 2021-09-18T15:08:31Z

What do you mean?

tananaev · 2021-09-18T15:18:56Z

I mean any ideas how to fix it via liquibase, so it's supported for add databases?

soshial · 2021-09-18T17:18:56Z

Looks like this should do the thing:

<modifyDataType tableName="tc_positions" columnName="fixtime" newDataType="TIMESTAMP(6)" />
<modifyDataType tableName="tc_positions" columnName="servertime" newDataType="TIMESTAMP(6)" />
<modifyDataType tableName="tc_positions" columnName="devicetime" newDataType="TIMESTAMP(6)" />
<modifyDataType tableName="tc_events" columnName="servertime" newDataType="TIMESTAMP(6)" />
<modifyDataType tableName="tc_devices" columnName="lastupdate" newDataType="TIMESTAMP(6)" />

How do check that it works?

tananaev · 2021-09-18T17:20:37Z

You have to check if it works with different database types. At least H2, MySQL, MS SQL and PostgreSQL.

soshial · 2021-09-18T17:30:06Z

Let's switch to another PR? (since it's a different bug fix)

How do I "launch" this new SQL changeset?

tananaev · 2021-09-18T17:32:31Z

Yep, makes sense to do a separate PR.

soshial · 2021-09-18T19:20:26Z

Besides this separate time issue, I would say everything works for me.

tananaev · 2021-09-18T22:12:46Z

src/main/java/org/traccar/handler/FilterHandler.java

    }

    private boolean filterZero(Position position) {
        return filterZero && position.getLatitude() == 0.0 && position.getLongitude() == 0.0;
    }

    private boolean filterDuplicate(Position position, Position last) {
-        if (filterDuplicate && last != null && position.getFixTime().equals(last.getFixTime())) {
+        if (filterDuplicate && last != null && position.getFixTime().getTime() == last.getFixTime().getTime()) {


Why is this needed?

If we're going to store milli/micro/nanoseconds in the TIMESTAMP field in the DB, then it means we would in fact compare float numbers. So maybe to filter out duplicates we should compare getTime() with a specified precision.

Sorry, where did you get floating numbers and nanoseconds? As far as I can tell, equals does exactly the same thing as you do manually:

https://docs.oracle.com/javase/8/docs/api/java/util/Date.html#equals-java.lang.Object-

Yes, currently it does the same thing. But, if we make this change #4746, then database would store time with milliseconds/nanosecond precision (as float). Hence, comparing new position (converted from text) and preceding (from DB) might not be equal in theory. That is why I suggest to do comparison with a certain precision. I can do additional commit for that. Do you agree?

I thought you planned to fix database accuracy?

Let's leave it as it is now. And deal with it in the separate PR.

Agree but this change should be reverted because it doesn't really do anything.

tsmgeek · 2021-09-19T15:20:06Z

Note with moving to fractional seconds you may need to account for BETWEEN queries (from UI etc) that may be off by 0.999999 seconds.
Ie between 00:01-00:02 when actually you may have meant 00:01.000000-00:02.999999.

In some apps at work we just use https://docs.oracle.com/javase/8/docs/api/java/time/LocalDateTime.html#MAX in code to always push max microseconds to account for this.

tananaev · 2021-09-28T18:00:55Z

src/main/java/org/traccar/handler/FilterHandler.java

-        Position last = null;
-        if (Context.getIdentityManager() != null) {
-            last = Context.getIdentityManager().getLastPosition(position.getDeviceId());
+        if (skipAttributes(position)) {


Looks like this actually changes the filtering logic. We only want to apply skipLimit and skipAttributes for certain types of filters.

Okay. Could you please elaborate? Why should those 2 skips work only for some filters?

There was a forum discussion about it if you want more context. Basically there are two categories of filtering. The ones that filter definitely invalid data (e.g. zero coordinates) and the ones that filter excessive data (e.g. duplicates). We always want to filter former, but want to keep latter if it has some important information (e.g. alarms).

Done. I reverted to the original logic and also preserved almost the same order of filter checks (as it is in current master branch).

tananaev · 2021-12-27T17:56:26Z

src/main/java/org/traccar/handler/FilterHandler.java

+
+        // filter out excessive data
+        long deviceId = position.getDeviceId();
+        if (filterDuplicate || filterDistance > 0 || filterMaxSpeed > 0 || filterMinPeriod > 0) {


What about filterStatic?

tananaev · 2021-12-27T18:21:26Z

Merged, thank you.

soshial · 2021-12-30T03:40:14Z

@tananaev: Could you please add the new filter.relative key to the documentation?

tananaev · 2021-12-30T04:01:26Z

The plan is to replace that documentation with something that's generated automatically.

add support for relative filtering

2a5bf04

soshial changed the title ~~Add support for relative filtering~~ Add support for relative filtering (#4735) Sep 10, 2021

soshial changed the title ~~Add support for relative filtering (#4735)~~ Add support for relative filtering Sep 10, 2021

tananaev reviewed Sep 10, 2021

View reviewed changes

filtering optimizations

d9ad819

tananaev reviewed Sep 12, 2021

View reviewed changes

improve naming and comments

7a3b106

tananaev reviewed Sep 12, 2021

View reviewed changes

setup/default.xml Outdated Show resolved Hide resolved

src/main/java/org/traccar/handler/FilterHandler.java Outdated Show resolved Hide resolved

src/main/java/org/traccar/handler/FilterHandler.java Outdated Show resolved Hide resolved

remove comments

fd91d6b

soshial force-pushed the relative_filtering branch from 0f8c43a to fd91d6b Compare September 13, 2021 06:27

tananaev reviewed Sep 13, 2021

View reviewed changes

fix SQL syntax in XML

a6ccfd6

tananaev reviewed Sep 13, 2021

View reviewed changes

improve SQL query

c345945

tananaev reviewed Sep 18, 2021

View reviewed changes

shorten IF clause

83cff6d

tananaev reviewed Sep 28, 2021

View reviewed changes

revert to the original filtering logic

29c63d9

tananaev reviewed Dec 27, 2021

View reviewed changes

filtering logic: fix static

41453ca

tananaev merged commit 790db62 into traccar:master Dec 27, 2021

Add support for relative filtering #4740

Add support for relative filtering #4740

Conversation

soshial commented Sep 10, 2021 • edited

Choose a reason for hiding this comment

soshial Sep 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soshial Sep 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tananaev left a comment

Choose a reason for hiding this comment

soshial commented Sep 13, 2021 via email • edited by tananaev

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tananaev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soshial commented Sep 18, 2021 • edited

tananaev commented Sep 18, 2021

soshial commented Sep 18, 2021 via email • edited by tananaev

tananaev commented Sep 18, 2021

soshial commented Sep 18, 2021 via email • edited by tananaev

tananaev commented Sep 18, 2021

soshial commented Sep 18, 2021

tananaev commented Sep 18, 2021

soshial commented Sep 18, 2021 • edited

tananaev commented Sep 18, 2021

soshial commented Sep 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmgeek commented Sep 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tananaev commented Dec 27, 2021

soshial commented Dec 30, 2021 • edited

tananaev commented Dec 30, 2021

soshial commented Sep 10, 2021 •

edited

soshial Sep 10, 2021 •

edited

soshial Sep 11, 2021 •

edited

soshial commented Sep 13, 2021 via email •

edited by tananaev

soshial commented Sep 18, 2021 •

edited

soshial commented Sep 18, 2021 via email •

edited by tananaev

soshial commented Sep 18, 2021 via email •

edited by tananaev

soshial commented Sep 18, 2021 •

edited

soshial commented Dec 30, 2021 •

edited