fix(pat-5034): include order-by expressions when computing the group-by clause #208
Conversation
Your org has enabled the Graphite merge queue for merging into mainYou must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. You can enable merging using labels in your Graphite merge queue settings. |
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
| ...(selection ?? []), | ||
| ...(orderBy ?? []) | ||
| .map((x: Ordering) => x[0]) | ||
| .filter((x) => !selectionMap.has(x)), |
There was a problem hiding this comment.
Can selectionMap.has(x) ever return true? Given how value equality works in Sets, which is pretty much like ===, it's not clear to me that the FieldExprs in selection could ever === the FieldExprs in the array that the filter is called on here.
Could you add a test that has equivalent, non-aggregateFieldExprs in the select and orderby, and see if they get deduped properly in the GroupbyList?
There was a problem hiding this comment.
In fact, the test case with avgPrice as the first order-by expression (see my comment below), results in a true value returned by selectionMap.has, and the group-by is correctly de-duped. Is there any reason that you think a non-aggregate field in the order-by that matches a selection expression will behave differently?
There was a problem hiding this comment.
Ajith's idea: Two derived fields would be unlikely to match.
There was a problem hiding this comment.
Good callout here @spencerwilson ! The derived field test did reveal the bug. Fixed it by using the field name in the selectionSet.
| const query = table | ||
| .select('id', 'name', price.avg().as('avgPrice')) | ||
| .filter(name.like('%bah%').and(createdOn.before(new Date('2023-01-01')))) | ||
| .orderBy(['avgPrice', 'DESC'], [createdOn, 'ASC']) // Note that createdOn is not in the seletion. |
There was a problem hiding this comment.
@spencerwilson , note the first orderBy expression. It will be mapped to the selection expression aliased to avgPrice. The group-by has no dupe for this expression.
There was a problem hiding this comment.
In any case, I'll include a non-agg that matches a selection in the orderBy.
… duped in grooup-by
| const grouping = expandedSelection.filter( | ||
| (fieldExpr) => !(fieldExpr instanceof AggregateFieldExpr) | ||
| ); | ||
| if (grouping) { |
There was a problem hiding this comment.
FYI grouping will always be truthy (though it may have grouping.length === 0)
Merge activity
|
Merge activity
|
graphite-app
bot
deleted the
ajith/pat-5034-order-by-non-aggs-in-group-by
branch
selection set when computing the group-by clause.
Test plan