Skip to content

fix: Update view version log timestamp for historical accuracy - #1218

Merged
liurenjie1024 merged 2 commits into
apache:mainfrom
c-thiel:ct/fix-view-version-log-history
Apr 18, 2025
Merged

liurenjie1024 merged 2 commits into
apache:mainfrom
c-thiel:ct/fix-view-version-log-history

Conversation

@c-thiel

@c-thiel c-thiel commented Apr 15, 2025 •

Copy link
Copy Markdown
Collaborator

What changes are included in this PR?

This change updates the behavior when setting a view version as current. When we set a previously existing view version as current, we now update its log timestamp to the current time rather than reusing the original timestamp of the ViewVersion.

Lets assume the following changes:

  1. ViewVersion 1 is added and set active
  2. View Version 2 is added and set active
  3. View Version 1 is set active.

This resulted in the following version_log:

    "version-log": [
        {
            "version-id": 1,
            "timestamp-ms": 1744716416168
        },
        {
            "version-id": 2,
            "timestamp-ms": 1744716449754
        },
        {
            "version-id": 1,
            "timestamp-ms": 1744716416168
        }
    ],

Note that the last and first entries have the same timestamp, namely the time stamp of the initial ViewVersion creation.

I believe this is undesired in a history, where we are interested when a certain change became active. It makes sense to use the exact timestamp of the ViewVersion if it was added in the same set of changes, but re-enabling a previously used view version (maybe years ago) should not add a history for this past timestamp.

Java behaviors the same way as rust currently does. @Fokko @nastra if we agree that we should change the behavior, we should also touch Java.

Are these changes tested?

Yes

@nastra

nastra commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

nice find @c-thiel and yes I agree that this seems like a bug that we should also fix on the Java side

@liurenjie1024 liurenjie1024 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.

Thanks @c-thiel for this fix, LGTM!

@liurenjie1024

Copy link
Copy Markdown
Contributor

Let's wait for a while for @Fokko 's response.

@sdd

sdd commented Apr 17, 2025

Copy link
Copy Markdown
Contributor

Agree, this seems like the desired behaviour.

@liurenjie1024
liurenjie1024 merged commit e497fb1 into apache:main Apr 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants