`--min-age` and `--max-age` should be computed relative to database session time

[nenb]

Describe the bug
I expect the --min-age and --max-age command-line arguments to be relative to the database session time, rather than relative to the time on the machine where data-diff is run (eg consider a database in a different time zone to a local machine running data-diff). I believe that this gives a user a potentially misleading impression on what data they have analysed with data-diff.

I think the easiest way to see this situation is via TestCLI in test_cli.py, by changing the timezone in the database session during the test setup (self.conn.query("SET @@session.time_zone='-01:00'")) :

class TestCLI(unittest.TestCase):
    def setUp(self) -> None:
        self.conn = get_conn(MySQL)
        self.conn.query("drop table if exists test_cli")
        self.conn.query("drop table if exists test_cli_2")
        table_src_name = "test_cli"
        table_dst_name = "test_cli_2"

        src_table = table(table_src_name, schema={"id": int, "datetime": datetime, "text_comment": str})
        self.conn.query(src_table.create())

        self.conn.query("SET @@session.time_zone='-01:00'")  # change time zone from +00:00
        db_time = self.conn.query("select now()", datetime)
        self.now = now = arrow.get(db_time)

        rows = [
            (now, "now"),
            (self.now.shift(seconds=-10), "a"),
            (self.now.shift(seconds=-7), "b"),
            (self.now.shift(seconds=-6), "c"),
        ]

        self.conn.query(src_table.insert_rows((i, ts.datetime, s) for i, (ts, s) in enumerate(rows)))
        _commit(self.conn)

        self.conn.query(f"CREATE TABLE {table_dst_name} AS SELECT * FROM {table_src_name}")
        _commit(self.conn)

        self.conn.query(src_table.insert_row(len(rows), self.now.shift(seconds=-3).datetime, "3 seconds ago"))
        _commit(self.conn)

    def tearDown(self) -> None:
        self.conn.query("drop table if exists test_cli")
        self.conn.query("drop table if exists test_cli_2")
        _commit(self.conn)

        return super().tearDown()

    def test_basic(self):
        diff = run_datadiff_cli(TEST_MYSQL_CONN_STRING, "test_cli", TEST_MYSQL_CONN_STRING, "test_cli_2")
        assert len(diff) == 1

    def test_options(self):
        diff = run_datadiff_cli(
            TEST_MYSQL_CONN_STRING,
            "test_cli",
            TEST_MYSQL_CONN_STRING,
            "test_cli_2",
            "--bisection-factor",
            "16",
            "--bisection-threshold",
            "10000",
            "--limit",
            "5",
            "-t",
            "datetime",
            "--max-age",
            "1h",  # can make test pass again by changing to 2h
        )
        assert len(diff) == 1

The tests should now fail, and can only be fixed by changing the --max-age value.

I've opened a draft PR (#283) with a sketch of a fix. If you think it has potential, please let me know what can be edited/improved, and then I will update the PR with test coverage (and possibly support for other databases if required).

[erezsh]

I agree, it was an oversight, and we should use the database time, and not the client time.

This applies to all databases, and we'll need to implement the "now()" function for every database we support, so it's better if I do it myself.

But as for improving the test coverage, that might be something you can help with.

[nenb]

But as for improving the test coverage, that might be something you can help with.

Sure!

This applies to all databases, and we'll need to implement the "now()" function for every database we support

In #283 I implemented this by adding a new method to AbstractDialect (new method = "provide SQL to get current timestamp from database session"). Each database supported can then override this method as required in their own implementation of BaseDialect. Did you have a different approach in mind? (Just curious!)

[erezsh]

Just small details.

• query_database_current_timestamp() isn't necessary
• current_timestamp() should return current_timestamp() without SELECT
• create ast/api for it in queries

Then db.query(now(), datetime) does exactly what db.query_database_current_timestamp() does. (and in future version, just db.query(now()) will be enough).

There are a lot of other small details, which is why I think it's easier just to do it myself.

[nenb]

That's reasonable. I'll close #283 then and let you implement a fix. Thanks for the extra details.

[erezsh]

Excuse the delay, I was in the middle of a big refactor that is now done.

@nenb I created a PR to fix this: #306 (branch issue284)

Can you please check and verify that it works as you'd expect?

[nenb]

@erezsh No problem.

I had a quick look, but I'm not sure that it's working as I expected.

If I run the same test as before with the database timezone modified (see the top of this issue for the exact details), the test still fails with the new changes. I would not expect the test to fail.

I will try to find some time over the next couple of days to explore further. In the meantime, I noticed some other things that might be relevant:

• what happens if both databases have different timezones? The current PR (I think) assumes that they are identical.
• does it matter which database the --min-age and --max-age are computed relative to? (Similar to the last question I guess.)
• poetry run unittest-parallel -j 16 was randomly failing for me until I stumbled on this issue that you were involved in, and released that I needed to pass the --class-fixtures flag to ensure correct set up and tear down. Is there a reason that you didn't include this in the docs etc? (If it's just something that was forgotten, I'm happy to open a PR updating the relevant sections)

[erezsh]

Hi @nenb ,

Good point, I will incorporate the test and make sure that it passes.

It also makes sense to compute the relative time for each database separately. However, I'm worried that in practice, it will end up causing confusing behavior.. If you copy the data from table A to table B, but it has a different timezone, or even just a different time, then using min-age or max-age would filter out different rows, and show differences that aren't really there. (since the data stays the same in both)

As for unittest-parallel, we solved that issue on our side. And anyway, it shouldn't fail "randomly". So something else is in play.
For what it's worth, I've been running it locally, on the CI, and others have to, without any problems, and without having to add the --class-fixtures flag.

[nenb]

Thanks @erezsh

And anyway, it shouldn't fail "randomly". So something else is in play.

I just had a quick check and it only seems to occur on the branch attached to the PR. I tested on Ubuntu 20.04 (run the unitttest-parallel command a few times and it fails sometimes and not others). I'll need to dig a bit deeper when I have the time.

It also makes sense to compute the relative time for each database separately. However, I'm worried that in practice, it will end up causing confusing behavior.

Actually, if the databases have different time/zones, then this will mean that data-diff won't really work, right? Because it uses the update column to determine differences, and so everything will appear as different in this situation? (Apologies if I have misunderstood the underlying logic.) Maybe there could be an early comparison of database time/zones, and if they are not equal then data-diff could exit with an explanation?

[erezsh]

Because it uses the update column to determine differences, and so everything will appear as different in this situation?

Presumably if the timestamps have been copied from one DB to another, their values and timezone should stay the same. Although I admit we haven't tested it, or paid much attention to this scenario.

Maybe there could be an early comparison of database time/zones, and if they are not equal then data-diff could exit with an explanation?

That's definitely the easiest solution. If we can't come up with a reasonable behavior, then perhaps that's what I'll do.

[erezsh]

Hello @nenb ,

The test had a fault that caused it to fail. Using arrow on the returned datetime added a UTC timezone when previously there wasn't one, causing the dates to be written to the db incorrectly (i.e. instead of being converted back to the session timezone).

But more importantly, the test can't really test data-diff, because setting the session timezone on one connection, doesn't affect the other connection (and running data-diff from the shell creates a new connection).

I think this is essentially untestable, unless I either

• Add a switch to the CLI to change the timezone, just for the tests
• Instead of running it in a shell, wrap everything in a function that can re-use a connection

Neither one feels like a good approach at the moment.

I wouldn't like to leave this feature untested, but I don't see an easy way out. But maybe I'm missing something obvious?

[erezsh]

As for different timezones in the databases when running data-diff, I think perhaps it's best to set the session timezone in both to UTC? That way we can be sure that equal dates produce no diff.