-
Notifications
You must be signed in to change notification settings - Fork 348
feat: Align date ranges to MV Granularity #1575
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
🦋 Changeset detectedLatest commit: 1829c1f The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR ReviewCritical IssuesNone found. The implementation looks solid. Code Quality ObservationsStrong Points:
Minor Suggestions (Non-blocking):
Summary✅ No critical issues found. The PR successfully:
The code follows HyperDX patterns and TypeScript best practices. Ready to merge after any final manual testing with the DDL provided in the PR description. |
E2E Test Results✅ All tests passed • 59 passed • 4 skipped • 748s
Tests ran across 4 shards in parallel. |
| "@types/ungap__structured-clone": "^1.2.0", | ||
| "@typescript-eslint/eslint-plugin": "^8.48.1", | ||
| "@typescript-eslint/parser": "^8.48.1", | ||
| "@ungap/structured-clone": "^1.3.0", |
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.
Added this polyfill for our tests, since the existing JSON.parse(JSON.stringify(obj)) workaround fails to correctly clone Dates (and other non-primitive types)
| return Object.values(Granularity).includes(value as Granularity); | ||
| }; | ||
|
|
||
| export function getAlignedDateRange( |
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.
Moved this and the related tests into common-utils, unchanged.
eacbf35 to
0fec287
Compare
teeohhem
left a comment
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.
Good stuff. Thanks!
Closes HDX-3124
Summary
This PR makes the following changes
EXPLAIN ESTIMATEquery results between the MV Optimization Indicator (the lightning bolt icon on each chart) and the chart itself. This roughly halves the number of EXPLAIN ESTIMATE queries that are made.Demo
Testing
To test locally with an MV, you can use the following DDL
DDL For an MV