Skip to content

Improve search queries based on datetime#226

Merged
adamruzicka merged 1 commit intowvanbergen:masterfrom
adamlazik1:fix-time-search
Feb 17, 2025
Merged

Improve search queries based on datetime#226
adamruzicka merged 1 commit intowvanbergen:masterfrom
adamlazik1:fix-time-search

Conversation

@adamlazik1
Copy link
Contributor

@adamlazik1 adamlazik1 commented Jan 14, 2025

The current implementation causes issues such as:

  1. When searching for timestamps greater than the given value and
    minutes and seconds are omitted the value, it returns only timestamps
    greater than value + 1 hour.

  2. When searching for timestamps greater than the given value and only
    month is provided as the smallest time unit, it excludes only timestamps
    from the first day of the month in the results but includes rest of
    month. Based on the logic implemented here this should not happen but
    instead the whole month should be excluded from the search results, i.e.
    '> January 2024' should exclude all January timestamps.

Same problems occur when searching with <=.

This PR aims to fix these issues.

@adamlazik1 adamlazik1 force-pushed the fix-time-search branch 2 times, most recently from ee12200 to 21d2a9b Compare January 15, 2025 16:30
@adamlazik1 adamlazik1 marked this pull request as ready for review January 15, 2025 16:30
Copy link
Collaborator

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Could we get a test or two for this?

@adamlazik1 adamlazik1 force-pushed the fix-time-search branch 2 times, most recently from 2e10a51 to 25baa82 Compare February 3, 2025 21:01
@adamlazik1
Copy link
Contributor Author

Tests added. Let me know whether you think this amount is sufficient or I should add some more.

@adamlazik1 adamlazik1 force-pushed the fix-time-search branch 2 times, most recently from 92ab406 to 1acabe6 Compare February 6, 2025 15:16
@adamlazik1
Copy link
Contributor Author

The test failures do not seem related to this PR unless I am mistaken. Can I do anything about them?

@adamruzicka
Copy link
Collaborator

Let me know whether you think this amount is sufficient or I should add some more.

This is fine. For a while I was entertaining the idea of having some which would also include custom time zones, but that isn't really necessary.

The test failures do not seem related to this PR unless I am mistaken. Can I do anything about them?

Most of them (except for jruby) should be dealt with in #227 , don't worry about them.

@adamruzicka
Copy link
Collaborator

Can I do anything about them?

Could you please rebase?

The current implementation causes issues such as:

1. When searching for timestamps greater than the given value and
minutes and seconds are omitted the value, it returns only timestamps
greater than value + 1 hour.

2. When searching for timestamps greater than the given value and only
month is provided as the smallest time unit, it excludes only timestamps
from the first day of the month in the results but includes rest of
month. Based on the logic implemented here this should not happen but
instead the whole month should be excluded from the search results, i.e.
'> January 2024' should exclude all January timestamps.

Same problems occur when searching with `<=`.

This PR aims to fix these issues.
@adamlazik1
Copy link
Contributor Author

rebased.

Copy link
Collaborator

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

Seems to do the trick

@adamruzicka adamruzicka merged commit 3242bb3 into wvanbergen:master Feb 17, 2025
12 of 18 checks passed
@adamruzicka
Copy link
Collaborator

Thank you @adamlazik1 !

@adamlazik1 adamlazik1 deleted the fix-time-search branch February 17, 2025 11:16
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.

2 participants