Move label values json aggregation into Java

[lampajr]

Changes proposed

I am proposing to move part of the /labelValues logic from jdbc to Java, especially the json aggregation.

This seems coming with a huge improvement, here some results I obtained:

Starting from empty db

Before

After

With prod backup and huge response (~100MB)

Before

After

Open points

• This PR is still in draft as with this change the filter and multiFilter does not work anymore as we don't have the whole object in the query anymore.. therefore we should think how we can fix that.
• Fix filter when it is a simple JSON path string
• Fix ordering by labels As agreed this won't be supported any longer

Check List (Check all the applicable boxes)

• My code follows the code style of this project.
• My change requires changes to the documentation.
• I have updated the documentation accordingly.
• All new and existing tests passed.

[lampajr]

This PR is still in draft as with this change the filter and multiFilter does not work anymore as we don't have the whole object in the query anymore.. therefore we should think how we can fix that.

As expected these tests are failing:

Error:  Failures: 
Error:    TestServiceTest.labelValuesFilterMultiSelectBoolean:582 1 expectation failed.
Expected status code <200> but was <500>.

Error:    TestServiceTest.labelValuesFilterMultiSelectNumber:555 1 expectation failed.
Expected status code <200> but was <500>.

Error:    TestServiceTest.labelValuesFilterMultiSelectStrings:528 1 expectation failed.
Expected status code <200> but was <500>.

[lampajr]

With the latest 2 commits I did:

• Moved the labelValues logic into a separated service - I think this is a great readability and maintainability improvement
• Fixed the filter and multiFilter when the provided filter is a JSON object (which showed a great improvement wrt the current master branch when using a filter - will share some results later)
• Applied the same improvement on RunService.labelValues call

There are still some TODOs:

• Fix the logic when the provided filter is a JSON path - @johnaohara @willr3 do we want to keep supporting this use case? If not I won't spend time on this - the only solution I see is to decompose the jsonpath and extracting the root.
• Fix the ordering by jsonpath - this is not working anymore as we have not the aggregated json in the query, @johnaohara @willr3 is this something we want to keep supporting? with this new implementation I don't even think this is still possible as this kind of ordering can be done in Java code once we aggregated JSON node - therefore we cannot use the psql jsonpath 🫤 As agreed this won't be supported any longer, just start/stop ordering other than the default by runId (or datasetId for labelValues for specific run)

[lampajr]

@willr3 @johnaohara I am not fully satisified about the dirty trick I had to do in order to decompose the jsonpath for the jsonpath filter but it looks working.

I did some other performance comparions between current master branch and this PR, I attached the main important ones here 👉 https://issues.redhat.com/browse/MPL-455.

I am still a bit surprised about the x10 improvement in the case of filter obj (I'd like to investigate a bit more to ensure the response is actually the same - it was in my local env).

Anyway, I think this PR is ready for review and it comes with good improvements IMO but happy to get any feedback!

[edit] I kept different commits to highlight the different changes, I will squash them before merging if no other changes are required

[johnaohara]

LGTM

[johnaohara]

@willr3 do you want to review this PR, looks good to me, but would be good to get a 2nd opinion as it is a big change to labelvalues

[willr3]

@willr3 do you want to review this PR, looks good to me, but would be good to get a 2nd opinion as it is a big change to labelvalues

taking a look

[willr3]

I think the only part we need to clarify at the moment is if this change still works with jsonpaths that start with a full depth search (e.g. $..name). If it does not then we need to prevent those from being used and return an appropriate error

[lampajr]

I rebased the PR and now some tests are failing, checking..

Fixed now!

[willr3]

LGTM

[johnaohara]

I just went to merge this PR, but realised that there is an implementation detail leaking from the API classes. I think we should fix that before merging

I was double checking that no API changes were required so that it could be back-ported

[johnaohara]

@lampajr thanks for sorting the api issue, please squash commits and I will merge

[lampajr]

@lampajr thanks for sorting the api issue, please squash commits and I will merge

Here we go, commits squashed @johnaohara 🚀