fix merging of groupBy subtotal spec results#8109
Conversation
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
It looks correct to me, my only comments are around making the code clearer & match up with the nice comment you wrote. I had some suggestions but I'm open to other ideas.
|
|
||
| return Sequences.withBaggage(result, () -> Lists.reverse(closeables).forEach(closeable -> CloseQuietly.close(closeable))); | ||
| } | ||
| }, subtotalQuery, null), |
There was a problem hiding this comment.
Something looks weird about this formatting…
There was a problem hiding this comment.
it was passing the intellij formatter , introduced few newlines and it looks better now.
| return GroupByRowProcessor.getRowsFromGrouper( | ||
| queryWithoutSubtotalsSpec, | ||
| subtotalSpec, | ||
| grouperSupplier |
There was a problem hiding this comment.
This is Grouper#1 right? Could you please add a comment where it's initialized, or change its name (grouperOneSupplier or something) to make the logic easier to follow?
There was a problem hiding this comment.
Or maybe breaking the method up would make it easier.
I guess what I'm trying to say is when reading, I was wishing it was more clear when we were using Grouper#1, when Grouper#2, and also who's responsible for closing what.
There was a problem hiding this comment.
renamed variable names to align them better with the comment, it should be easier to follow now.
I thought of breaking the method and then realized most of it is actually method calls which are big because of the number of arguments they take. take a look now, if it still looks difficult then I can try to break it.
| Sequence<Row> queryResult | ||
| ) | ||
| { | ||
| // How it works? |
There was a problem hiding this comment.
Thanks for writing this comment, it helped.
| Sequence<Row> result = GroupByRowProcessor.getRowsFromGrouper( | ||
| subtotalQuery, | ||
| subtotalSpec, | ||
| Suppliers.memoize(() -> GroupByRowProcessor.createGrouper( |
There was a problem hiding this comment.
I believe this is Grouper#2. Could you please make it clear via comment, naming, or splitting out into a separate method?
himanshug
force-pushed
the
bugfix
branch
3 times, most recently
from
968ecf7 to
7c2efc5
Compare
|
strict build is broken due to #8138 |
|
@gianm can you please review it again ? |
2 participants
Fixes #8091
Issue for Future Optimization #8108
This PR has: