-
Notifications
You must be signed in to change notification settings - Fork 969
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aggregation support for ledger dumps #3490
Conversation
@@ -2,31 +2,32 @@ | |||
// under the Apache License, Version 2.0. See the COPYING file at the root | |||
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 | |||
|
|||
#include "util/xdrquery/XDRMatcher.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong commit? I see that you have a "Rename XDRMatcher.h/cc to XDRQuery to be more general" later on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
@@ -546,7 +546,7 @@ exit /b 0 | |||
<ClCompile Include="..\..\src\util\test\SchedulerTests.cpp" /> | |||
<ClCompile Include="..\..\src\util\XDRCereal.cpp" /> | |||
<ClCompile Include="..\..\src\util\xdrquery\test\XDRQueryTests.cpp" /> | |||
<ClCompile Include="..\..\src\util\xdrquery\XDRMatcher.cpp" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this commit should probably be the first in your PR as the other commits seem to depend on the new name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did the rename after all the changes, but I guess this got messed up when I was squashing some intermediate commits. Updated the history.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you need to refresh timestamps for commits as to not confuse Github: right now that commit looks like the last one in the list but it's the first one in your chain.
The way to do this, is to rebase by running git rebase --force-rebase --exec "sleep 2" --exec "git commit --amend --date=now" master
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rename is supposed to be the last commit and it seems like it is both in git hub and git logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes, I saw that 🤦♂️ . In my mind you were going to rename first before making the other changes. Looks good
…ction. This allows to support group by and aggregation in `dump-ledger` command.
The output is dumped to CSV as it's always flat. I'm not adding JSON format for this for the sake of simplicity. Also updated the JSON dump format to be more `jq`-friendly.
r+ 2b1dce5 |
Description
This adds support of group by and some aggregation operators for simple aggregation operations.
Also added output format improvements for JSON dumps and more docs/examples.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)