fix(search_family): Remove the output of extra fields in the FT.AGGREGATE command #4231
Conversation
BagritsevichStepan
force-pushed
the
df.search/fix-aggregate-steps-result
branch
from
8affbf7 to
c108b12
Compare
| }; | ||
| } | ||
|
|
||
| PipelineResult Process(std::vector<DocValues> values, absl::Span<const PipelineStep> steps) { | ||
| PipelineResult Process(std::vector<DocValues> values, |
There was a problem hiding this comment.
Please put your prod-duty hat, and think about all the VLOG statements you would like to have, if we have problems in production with this code.
| return result; | ||
| values = std::move(result.value()); | ||
| PipelineResult step_result = step(std::move(result)); | ||
| result = std::move(step_result); |
There was a problem hiding this comment.
I would prefer to see step method modifying the result instead of all these moves
There was a problem hiding this comment.
Do you mean that we need to refactor this logic and have something like Aggregator that stores current result and has methods like DoSort, DoReduce? Or use pointer here
There was a problem hiding this comment.
What about this question? to refactor steps and modify the result instead of moving it every time
There was a problem hiding this comment.
Yes, I spoke about it today. I'm finalizing it
There was a problem hiding this comment.
Are you going to create another PR?
There was a problem hiding this comment.
Yes, I think that it would be better to create another PR
There was a problem hiding this comment.
Also for logs that Roman mentioned, because we first need to refactor the code before adding the logs in the appropriate places
…GATE command fixes dragonflydb#4230 Signed-off-by: Stepan Bagritsevich <[email protected]>
Signed-off-by: Stepan Bagritsevich <[email protected]>
BagritsevichStepan
force-pushed
the
df.search/fix-aggregate-steps-result
branch
from
c108b12 to
49718c1
Compare
|
Just fixed the nit |
If you address comments please do an additional commit instead of amend and force push, because it's easier to review. |
fixes #4230
First PR for complete support of the SORTBY option (#3631)
To see examples please check the issue.
Add code that checks, during aggregate steps, what fields should be printed.