-
Notifications
You must be signed in to change notification settings - Fork 113
Add paging support for C++ metrics #784
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
Conversation
mcserep
left a comment
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.
Nice approach on avoiding code redundancy in the service implementation! 👏
The Thrift CppMetricsService was missing JavaScript bindings, I think this is a mistake so I added that as well to plugins/metrics/service/CMakeLists.txt.
I think it was simply missing, because the frontend does not support metrics yet. It can be added though.
|
@wbqpk3 The CI fails to build on Ubuntu 20.04. While we should drop support for Ubuntu 20.04 soon, now it should be checked why the build fails there, otherwise the Docker images won't be build too in the pipeline. |
|
Looking at the build logs, it seems the error is caused by the switch to the C++ 17 standard. |
|
I just also noticed a warning message that Ubuntu 20.04 runner images on GitHub will no longer be supported after 2025-04-15. |
Yes, and while it is not April 15th yet, CI jobs for Ubuntu 20.04 already started to skip, due to image unavailability. I have disabled the CI build for Ubuntu 20.04 in b9e6733, and added an issue #789 to deprecate it completely.
Thanks for attempting to resolve it, but in this case it is not necessary anymore and simply reverting 77566fe would be better. |
77566fe to
95e0c34
Compare
|
@mcserep Removed the Ubuntu 20.04 CI commit and rebased to master. |
This PR implements paged versions of existing functions in
CppMetricsService:getPagedCppAstNodeMetricsForPathgetPagedCppAstNodeMetricsDetailedForPathgetPagedCppFileMetricsForPathI also upgraded the C++ standard to 17, since I used
if constexprin a function template.The Thrift
CppMetricsServicewas missing JavaScript bindings, I think this is a mistake so I added that as well toplugins/metrics/service/CMakeLists.txt.On a high level, the paging is implemented as follows:
DISTINCTastNodeIdfrom tableCppAstNodeMetrics.path(usingLIKE) and also forpageSizeandpageNumber(usingLIMITandOFFSET).astNodeIdwith exact size aspageSize. We runSELECTonCppAstNodeMetricsonce again, selecting all rows where theastNodeIdis in the list (using SQLINkeyword).The PR was also tested using Thrift JS bindings.