New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: filter datetime (sqlite / mysql) #8385
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughWalkthroughThe recent updates to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
packages/nocodb/src/db/conditionV2.ts (1)
Line range hint
1-1000
: Review the overall structure and logic of theparseConditionV2
function.The
parseConditionV2
function is quite large and handles multiple responsibilities, which could be split into smaller, more manageable functions. This would improve readability and maintainability. Consider refactoring to separate the logic for different types of filters and their handling into distinct functions or modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good and I have few doubts, will test locally couple of scenarios and then I will proceed with merge.
@@ -740,7 +759,9 @@ const parseConditionV2 = async ( | |||
// else | |||
qb = qb.where(knex.raw('??::date = ?', [field, val])); | |||
} else { | |||
qb = qb.where(knex.raw('DATE(??) = DATE(?)', [field, val])); | |||
qb = qb.where( | |||
knex.raw("DATE(??, 'localtime') = DATE(?)", [field, val]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does all databases(sqlite, mysql and mssql) take localtime
as second argument ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MySQL was excluded from the beginning. Here, I am only testing SQLite and have not considered MSSQL. I can modify this through conditional statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can introduce an if statement to avoid any unexpected behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only fixes the issue. I feel this part can be abstracted and optimized further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/nocodb/src/db/conditionV2.ts (1)
Line range hint
600-684
: Ensure consistent handling of timezone and date boundaries.The changes made to handle dates more accurately by considering timezone and the start or end of the day are crucial. However, the logic is quite complex and could benefit from being broken down into smaller, more manageable functions. This would improve readability and maintainability. Consider refactoring the date manipulation logic into separate functions that handle the conversion and boundary setting.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/src/db/conditionV2.ts (4 hunks)
Additional comments not posted (1)
packages/nocodb/src/db/conditionV2.ts (1)
763-764
: Address potential timezone issues for SQLite date comparisons.This change is crucial for ensuring that SQLite handles dates correctly in different timezones. Please verify that this adjustment behaves as expected across different timezone settings.
Verification successful
The search results indicate that the specific pattern
DATE(??, 'localtime') = DATE(?)
is used inpackages/nocodb/src/db/conditionV2.ts
. This confirms that the code is attempting to handle local time for date comparisons in SQLite.
- File:
packages/nocodb/src/db/conditionV2.ts
- Line: 763-764
This usage ensures that the date comparison is done in local time, which addresses potential timezone issues for SQLite date comparisons.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript $'DATE(??, \\'localtime\\') = DATE(?)'Length of output: 148
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript 'DATE\(\?\?, \'localtime\'\) = DATE\(\?\)'Length of output: 93
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript "DATE\(??, 'localtime'\) = DATE\(\?\)"Length of output: 102
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg "DATE\(??, 'localtime'\) = DATE\(\?\)"Length of output: 49
Script:
#!/bin/bash # Description: Verify the usage of knex.raw for date comparisons in the codebase. # Test: Search for the usage of knex.raw in the codebase to understand how date comparisons are handled. rg "knex.raw"Length of output: 24434
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of