-
Notifications
You must be signed in to change notification settings - Fork 250
fix: aggFn use default instead of null #782
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: aggFn use default instead of null #782
Conversation
🦋 Changeset detectedLatest commit: cb240c2 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 Git ↗︎
|
Switched to the -OrDefault version of the float conversion when combined with the aggregate functions to prevent emitting null. Ref: HDX-1379
afde3ce
to
b4891e7
Compare
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.
Pull Request Overview
This PR fixes an issue with numeric aggregate functions by switching from a null conversion approach to one that uses the type’s default value. Key changes include updating the unsafe SQL expression in the chart config rendering, adding the executeSqlCommand helper in the API package, and introducing new tests for the aggFn functionality.
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/common-utils/src/renderChartConfig.ts | Updated the SQL expression conversion from toFloat64OrNull to toFloat64OrDefault |
packages/api/src/fixtures.ts | Added a helper function executeSqlCommand for executing SQL commands against ClickHouse |
packages/api/src/clickhouse/tests/renderChartConfig.test.ts | Introduced tests for numeric aggregate functions that now use default values |
.changeset/curly-ducks-jump.md | Documented the changes for numeric aggregates in the changeset |
Files not reviewed (2)
- packages/api/src/clickhouse/tests/snapshots/renderChartConfig.test.ts.snap: Language not supported
- packages/common-utils/src/tests/snapshots/renderChartConfig.test.ts.snap: Language not supported
Switched to the
-OrDefault
version of the float conversion when combined with the aggregate functions to prevent emitting null.Ref: HDX-1379