Fixes #45, strip sorts from weight check queries #46
Conversation
cdeszaq
force-pushed
the
fixSortingOnWeightCheckQueries
branch
from
1c4432c to
cad78fe
Compare
| } | ||
|
|
||
| public LimitSpec withLimit(OptionalInt limit) { | ||
| return new LimitSpec(this.columns, limit); |
There was a problem hiding this comment.
this is unnecessary in both withers.
| ((GroupByQuery) innerQuery).getLimitSpec() | ||
| ((GroupByQuery) innerQuery).getLimitSpec() == null ? | ||
| null : | ||
| ((GroupByQuery) innerQuery).getLimitSpec().withColumns(new LinkedHashSet<>()) |
There was a problem hiding this comment.
Seeing as how we're doing this twice, and all the casting makes for a fairly complex statement, I would suggest extracting this into a helper method:
private LimitSpec extractLimitSpec(GroupByQuery query) {
return query.getLimitSpec() == null ? null : query.getLimitSpec().withColumns(new LinkedHashSet());
} This will simplify the constructor call to:
...
query.getQueryType() == QueryType.GROUP_BY ? extractLimitSpec((GroupByQuery) query) : nulland this code to:
...
extractLimitSpec((GroupByQuery) innerQuery)It also means that we can have a positive condition, which is always nice.
There was a problem hiding this comment.
You could also have it take a Query instead, and do the type checking and casting inside the method. Either approach is fine.
| GroupByQuery groupByQuery = builder.buildQuery(apiRequest, merger.merge(apiRequest)) | ||
|
|
||
| expect: "The groupBy query has a sort, as a pre-condition" | ||
| !groupByQuery.limitSpec.columns.empty |
There was a problem hiding this comment.
Since columns is a collection, you can simplify this using groovy truthiness:
groupByQuery.limitSpec.columns|
|
||
| and: "The weight-check query doesn't have a sort at either level" | ||
| new WeightEvaluationQuery(groupByQuery, 1).limitSpec.columns.empty | ||
| new WeightEvaluationQuery(groupByQuery, 1).innermostQuery.limitSpec.columns.empty |
There was a problem hiding this comment.
You can do something similar here.
|
👍 |
| } | ||
|
|
||
| public LimitSpec withLimit(OptionalInt limit) { | ||
| return new LimitSpec(this.columns, limit); |
There was a problem hiding this comment.
It can underline which parameters are pulled from local instance.
|
I'm warm to andrew's requests. Otherwise 👍 for me. |
cdeszaq
force-pushed
the
beginDevelopmentOnV0-6-x
branch
from
b2f91f3 to
ebe14e9
Compare
cdeszaq
force-pushed
the
fixSortingOnWeightCheckQueries
branch
from
cad78fe to
d2a7bef
Compare
cdeszaq
force-pushed
the
beginDevelopmentOnV0-6-x
branch
from
ebe14e9 to
bd50d01
Compare
cdeszaq
force-pushed
the
fixSortingOnWeightCheckQueries
branch
from
d2a7bef to
6401722
Compare
cdeszaq
added
MERGEABLE
NEED SQUASH
and removed
NEED 1 REVIEW
NEED CHANGES
REVIEWABLE
labels
cdeszaq
force-pushed
the
fixSortingOnWeightCheckQueries
branch
from
4cf7a3d to
b2141c1
Compare
cdeszaq
force-pushed
the
fixSortingOnWeightCheckQueries
branch
from
b2141c1 to
ef3c0eb
Compare
|
Merged |
Note: Based on other PRs. Once those merge, this PR will be repointed to master.