Mark sorting error in longRowProcessor as downstream

[idastambuk]

What this PR does / why we need it:
We've been getting plugin errors reported for Athena (and some other plugins): long series must be sorted ascending by time to be converted: Could not process SQL results. This seems like a downstream/user error, but lmk if that's not the case.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

[gabor]

hi,

i understand the point, but i would recommend not to mix errorsource into this place.
what do you think about this alternative:

• the caller (athena in this case), can check the returned error, and if it this error, it can mark it as downstream

but right now it's hard to detect this specific error, so i recommend doing a change in this place, something like this:

var ErrorSeriesUnsorted = errors.new("long series must be sorted ascending by time to be converted");

.
.
.
func process() {
    .
    .
    .
    if .... {
        return ErrorSeriesUnsorted;
    }
}

and then in the athena datasource you can do errors.Is() to check the returned error, and if it is this error, mark it.

alternatively, you may consider any error returned from process() as a downstream error.. i'm not sure, did not think too deeply about it, but it may be right.

[ivanahuckova]

I haven't looked much at the code, but isn't this actually our error? If we want to process result and we require it to be sorted, shouldn't we sort it before processing them?

[kylebrandt]

I haven't looked much at the code, but isn't this actually our error? If we want to process result and we require it to be sorted, shouldn't we sort it before processing them?

Sorting can be resource intensive, and generally databases will do it more efficiently - so I think it is better to suggest that the user sort the data (although in the FE case that processing falls on the browser).

[ivanahuckova]

Sorting can be resource intensive, and generally databases will do it more efficiently...

@kylebrandt Then we should probably improve the error to something like Unable to process the data because it’s not sorted in ascending order by time. Please updated your query to sort the data by time and try again. What do you think?

i understand the point, but i would recommend not to mix errorsource into this place...

@gabor, I see your point, but also I am wondering what would be the downside or marking it as downstrem in here. I fully agree and like var ErrorSeriesUnsorted = errors.new("long series must be sorted ascending by time to be converted"); and I think we should implement error that way, but what about still wrapping it with downstream error so all plugins don't have to check for it, cause we have many plugins where we use this processing and we would have to check it everywhere individually.

[kylebrandt]

Unable to process the data because it’s not sorted in ascending order by time. Please updated your query to sort the data by time and try again. What do you think?

I think that is better, helps guide users to fix.

[ivanahuckova]

I did a follow up in #1134

[gabor]

@ivanahuckova

i understand the point, but i would recommend not to mix errorsource into this place...

@gabor, I see your point, but also I am wondering what would be the downside or marking it as downstrem in here. I fully agree and like var ErrorSeriesUnsorted = errors.new("long series must be sorted ascending by time to be converted"); and I think we should implement error that way, but what about still wrapping it with downstream error so all plugins don't have to check for it, cause we have many plugins where we use this processing and we would have to check it everywhere individually.

hi (sorry for the delay). i think the problem is, it's not guaranteed the error is downstream. so, this functionality is happening in the LongToWide function, which simply converts dataframes from long-format to wide-format. it is completely possible, that a datasource-plugin will take some data, manually sort them, and then call this thing.

in other words, the error is downstream in the way that the error is not the fault of LongToWide, but the fault of the caller of LongToWide. but, it can still be a plugin-error (when, for example, the plugin should be ensuring the data is sorted). WDYT?