fix: area stats query on number columns with empty strings #256
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an error that occurred when querying area statistics on number columns containing empty strings. The issue was discovered when displaying AB Charitable Trust clusters on the map.
Key Changes:
- Modified the SQL query in
getColumnValueByAreato handle empty strings in number columns by converting them to '0' before casting to float - Added
COALESCE(NULLIF(...))pattern to prevent PostgreSQL casting errors on empty strings
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| )} | ||
|
|
||
| {viewConfig.calculationType !== CalculationType.Count && | ||
| columnOneIsNumber && ( |
There was a problem hiding this comment.
The visibility condition for this switch differs from the "Aggregation" select above (lines 300-305). The aggregation select checks for dataRecordsWillAggregate and areaSetGroupCode, but this switch doesn't. This inconsistency could cause the switch to appear in scenarios where it's not applicable or meaningful. Consider adding the same conditions to ensure the switch is only shown when it will actually affect the behavior.
| columnOneIsNumber && ( | |
| columnOneIsNumber && | |
| dataRecordsWillAggregate( | |
| dataSource?.geocodingConfig, | |
| viewConfig.areaSetGroupCode, | |
| ) && ( |
| }) { | ||
| const { viewConfig } = useMapViews(); | ||
| const choroplethDataSource = useChoroplethDataSource(); | ||
| const [, setHoverArea] = useHoverArea(); |
There was a problem hiding this comment.
This change adds a setHoverArea handler that wasn't previously imported. While the onClose handler at line 108 is a reasonable addition, this change appears unrelated to the PR's stated purpose of fixing area stats queries with empty strings. Consider moving this to a separate PR focused on popup behavior improvements.
| longitude={coordinates[0]} | ||
| latitude={coordinates[1]} | ||
| closeButton={false} | ||
| onClose={() => setHoverArea(null)} |
There was a problem hiding this comment.
This change adds an onClose handler that appears unrelated to the PR's stated purpose of fixing area stats queries with empty strings. Consider moving this to a separate PR focused on popup behavior improvements.
| onClose={() => setHoverArea(null)} |
This bug noticed when trying to display the AB Charitable Trust clusters on the map.
We may need to revisit this in the future if we have users who want
""to be treated as "No data" instead of0, but this hasn't happened yet.