-
Notifications
You must be signed in to change notification settings - Fork 715
fix: histogram issue with order by asc #4606
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
Conversation
WalkthroughThe changes in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant useLogs
participant fnParsedSQL
participant searchObj
User->>useLogs: Call useLogs()
useLogs->>fnParsedSQL: Call fnParsedSQL()
fnParsedSQL-->>useLogs: Return parsedSQL
useLogs->>searchObj: Access partitionDetail
searchObj-->>useLogs: Return partitions
useLogs->>useLogs: Check isTimestampASC(parsedSQL?.orderby)
alt If timestamps are ascending
useLogs->>useLogs: Reverse partitions
end
useLogs->>useLogs: Check start and end time match
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 1
Outside diff range and nitpick comments (1)
web/src/composables/useLogs.ts (1)
Line range hint
2404-2430: Simplify and secure date manipulationsThe code introduces complex manual date manipulations, which can be error-prone and may not handle time zones and edge cases correctly. Consider using a date utility library like
date-fnsormomentto simplify the logic and ensure accurate date calculations.For example, using
date-fnsto handle date calculations:+ import { fromUnixTime, formatISO, addSeconds, subSeconds } from "date-fns"; ... if (partitions[0][0] == queryReq.query.start_time && partitions[0][1] == queryReq.query.end_time) { histogramResults = []; - let date = new Date(); - const startDateTime = searchObj.data.customDownloadQueryObj.query.start_time / 1000; - const endDateTime = searchObj.data.customDownloadQueryObj.query.end_time / 1000; - const nowString = res.data.hits[0].zo_sql_key; - const now = new Date(nowString); - const day = String(now.getDate()).padStart(2, "0"); - const month = String(now.getMonth() + 1).padStart(2, "0"); - const year = now.getFullYear(); - - const dateToBePassed = `${day}-${month}-${year}`; - const hours = String(now.getHours()).padStart(2, "0"); - let minutes = String(now.getMinutes()).padStart(2, "0"); - if (searchObj.data.histogramInterval / 1000 <= 60000) { - minutes = String(now.getMinutes() + 1).padStart(2, "0"); - } - const time = `${hours}:${minutes}`; - const currentTimeToBePassed = convertDateToTimestamp( - dateToBePassed, - time, - 'UTC' - ); - for ( - let currentTime = currentTimeToBePassed.timestamp / 1000; - currentTime < endDateTime; - currentTime += searchObj.data.histogramInterval / 1000 - ) { - date = new Date(currentTime); - histogramResults.push({ - zo_sql_key: date.toISOString().slice(0, 19), - zo_sql_num: 0, - }); - } - for ( - let currentTime = currentTimeToBePassed.timestamp / 1000; - currentTime > startDateTime; - currentTime -= searchObj.data.histogramInterval / 1000 - ) { - date = new Date(currentTime); - histogramResults.push({ - zo_sql_key: date.toISOString().slice(0, 19), - zo_sql_num: 0, - }); - } + const startDateTime = fromUnixTime(searchObj.data.customDownloadQueryObj.query.start_time / 1e6); + const endDateTime = fromUnixTime(searchObj.data.customDownloadQueryObj.query.end_time / 1e6); + const intervalSeconds = searchObj.data.histogramInterval / 1e6; + let currentTime = new Date(res.data.hits[0].zo_sql_key); + + // Generate future histogram intervals + while (currentTime < endDateTime) { + histogramResults.push({ + zo_sql_key: formatISO(currentTime, { representation: 'complete' }), + zo_sql_num: 0, + }); + currentTime = addSeconds(currentTime, intervalSeconds); + } + + // Reset to the original time and generate past histogram intervals + currentTime = new Date(res.data.hits[0].zo_sql_key); + while (currentTime > startDateTime) { + currentTime = subSeconds(currentTime, intervalSeconds); + histogramResults.push({ + zo_sql_key: formatISO(currentTime, { representation: 'complete' }), + zo_sql_num: 0, + }); + } }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- web/src/composables/useLogs.ts (1 hunks)
Additional comments not posted (2)
web/src/composables/useLogs.ts (2)
2401-2403: Verify the logic for partition reversalThe condition checks if the timestamps are in ascending order and reverses the
partitionsarray accordingly. Ensure that this reversal is necessary and that it aligns with the expected data processing flow.
Line range hint
2413-2427: Prevent potential infinite loops in histogram generationEnsure that the loop conditions in the histogram generation logic will eventually terminate. Verify that
currentTimeprogresses correctly to avoid infinite loops, especially when dealing with time intervals.
| const partitions = JSON.parse( | ||
| JSON.stringify( | ||
| searchObj.data.queryResults.partitionDetail.partitions, | ||
| ), | ||
| ); |
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.
Use a reliable deep cloning method instead of JSON.parse(JSON.stringify(...))
Using JSON.parse(JSON.stringify(...)) for deep cloning can lead to issues with functions, undefined values, or circular references. It may also affect performance. Consider using a utility like lodash.cloneDeep for a more robust deep cloning process.
Apply this diff to import cloneDeep from Lodash and use it:
+ import { cloneDeep } from "lodash-es";
...
- const partitions = JSON.parse(
- JSON.stringify(
- searchObj.data.queryResults.partitionDetail.partitions,
- ),
- );
+ const partitions = cloneDeep(searchObj.data.queryResults.partitionDetail.partitions);Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
These changes will lead to more reliable data representation in the application.