fix: DH-13522: exclude some aggs for arrays#2697
Conversation
There was a problem hiding this comment.
Pull request overview
Updates aggregation validation to treat array-typed columns as unsupported for distinct-style aggregations, preventing errors by excluding those columns from CountDistinct, Distinct, and Unique operations.
Changes:
- Added
TableUtils.isArrayTypehelper (and unit tests) to detect[]-style array column types. - Updated
isValidOperationto disallowCOUNT_DISTINCT,DISTINCT, andUNIQUEfor array columns (while leavingCOUNT,FIRST,LASTvalid for any type). - Added dedicated unit tests for aggregation operation/type validity in
iris-grid.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/jsapi-utils/src/TableUtils.ts | Adds isArrayType helper used to gate operations on array-typed columns. |
| packages/jsapi-utils/src/TableUtils.test.ts | Adds unit coverage for isArrayType. |
| packages/iris-grid/src/sidebar/aggregations/AggregationUtils.ts | Excludes distinct-style aggregations for array column types via isValidOperation. |
| packages/iris-grid/src/sidebar/aggregations/AggregationUtils.test.ts | Adds unit tests asserting validity rules for operations vs column types, including arrays. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2697 +/- ##
==========================================
+ Coverage 50.43% 50.72% +0.28%
==========================================
Files 788 788
Lines 44909 44912 +3
Branches 11442 11442
==========================================
+ Hits 22652 22782 +130
+ Misses 22238 22111 -127
Partials 19 19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jnumainville
requested review from
a team,
SimonVutov,
bmingles,
ethanalvizo and
mattrunyon
and removed request for
a team,
SimonVutov,
ethanalvizo and
mattrunyon
| ]; | ||
|
|
||
| it.each(ops)('%s returns true for any column type', op => { | ||
| expect(isValidOperation(op, 'int')).toBe(true); |
There was a problem hiding this comment.
any reason these are single type checks instead of using the type arrays defined at top of test?
| ]; | ||
|
|
||
| it.each(ops)('%s returns true for non-array types', op => { | ||
| expect(isValidOperation(op, 'int')).toBe(true); |
There was a problem hiding this comment.
any reason these are single type checks instead of using the type arrays defined at top of test?
There was a problem hiding this comment.
Same general question for any other tests that only target a single value in cases where we expect all in an array to match.
bmingles
left a comment
bmingles
left a comment
There was a problem hiding this comment.
Looks good to me overall. Left a few questions on unit test.
CountDistinct, Distinct, Unique aggs now are empty for array columns instead of throwing an error