Skip to content

Commit

Permalink
fix: correctly handle merging of domains with more than one sort obje…
Browse files Browse the repository at this point in the history
…ct (#8567)

* Adds a fix for sort bug with layers

* Updates test case name
  • Loading branch information
MufaroMakiwa authored Feb 13, 2023
1 parent 749010d commit 1eedb8f
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
8 changes: 7 additions & 1 deletion src/compile/scale/domain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,13 @@ export function mergeDomains(domains: VgNonUnionDomain[]): VgDomain {
let sort = sorts[0];
if (sorts.length > 1) {
log.warn(log.message.MORE_THAN_ONE_SORT);
sort = true;
// Get sorts with non-default ops
const filteredSorts = sorts.filter(s => isObject(s) && 'op' in s && s.op !== 'min');
if (sorts.every(s => isObject(s) && 'op' in s) && filteredSorts.length === 1) {
sort = filteredSorts[0];
} else {
sort = true;
}
} else {
// Simplify domain sort by removing field and op when the field is the same as the domain field.
if (isObject(sort) && 'field' in sort) {
Expand Down
32 changes: 32 additions & 0 deletions test/compile/scale/domain.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1084,6 +1084,38 @@ describe('compile/scale', () => {
})
);

it(
'should use non-min aggregation when sort ops conflict',
log.wrap(localLogger => {
const domain = mergeDomains([
{
data: 'foo',
field: 'a',
sort: {
op: 'min'
}
},
{
data: 'foo',
field: 'a',
sort: {
op: 'sum'
}
}
]);

expect(domain).toEqual({
data: 'foo',
field: 'a',
sort: {
op: 'sum'
}
});

expect(localLogger.warns[0]).toEqual(log.message.MORE_THAN_ONE_SORT);
})
);

it(
'should warn if sorts conflict even if we do not union',
log.wrap(localLogger => {
Expand Down

0 comments on commit 1eedb8f

Please sign in to comment.