Adding sparkline aggregate function - #141388

what's this 0, doing here?

This def is to define a quinary function (with inputs trend aggregate function, timestamp field, bucket count, from date, to date) and the 0 represents the number of optional parameters. All of the inputs should be required so I believe this is what we'd want here.

Hmm... the other similar methods don't have this 0. Anyway, not your problem...

This does seem pretty wild.

Is there something different we should be doing here instead?

This does seem pretty wild.

I'm not sure if we need these still given that we removed FromPartial/ToPartial for a version?

I don't think we need this, yeah. If we don't serialize then we don't need it.

To clarify there used to be some checks here for various transport versions. It sounds like you're saying that I should just remove this function entirely and I assume likely remove ToPartial and FromPartial from the AggregateWriteables then? Is it just because we used to serialize these when they were available for users to call directly and now we won't need that?

I believe at the moment this isn't serialized at all, right? You could do:

if (this == PARTIAL_AGG) { throw new IllegalStateException("never serialized"); }

and all the tests would still pass, right? I'm not suggestion you do, just checking.

I think it's important to have proper SupportedVersion on these, just in case someone tries to serialize this. That way they get a reasonable error message.

Previously we didn't need this. In fact, we send partial aggregations right now over the wire, but we do so with a weird type hack that is from long, long, long, long ago. It's worth at least a comment about how this differs.

Also! Could you explain why we need this now? I'm not surprised that we do, but it'd be nice to know for sure.

Is it just because FromPartial/ToPartial did it?

I think you answered this below but I believe we use this data type as a hack for the type checker to handle the intermediate state of these aggregates. From your comments above it sounds like we don't need to be able to serialize this if it's not being offered as a function for users. Given these facts what do you recommend for the supported version here?

👍

We've got a series of failing tests (the bwc-snapshot ones) that fail on AllSupportedFieldsIT as it's trying to serialize PARTIAL_AGG in an older version where this is not supported. I've removed the serialization logic as we said below we don't need it but I think we maybe need to change the supportedOnAllNodes() here to something else to stop the tests from trying to serialize the type? Any recommendation on how to handle this?

Without the serialization logic in ToPartial/FromPartial we also see errors in MixedClusterEsqlSpecIT as I think it's running into ToPartial/FromPartial's and they are not registered as NamedWriteables. Does this mean we do need the serialization logic?

An update to the comment above. It seems the AllSupportedFieldsIT test failures are not due to serialization issues but rather just that we need to exclude PARTIAL_AGG as a type generated in the AllSupportedFieldsTestCase.supportedInIndex().

The question about what serialization logic is required is still one I need some advice on. I'm not quite clear as the Aggregate class extends NamedWriteable which implies ToPartial/FromPartial can be serialized and it seems removing serialization code causes issues. I'm not sure if we need to just include this code with some exceptions to prevent serialization or keep the code omitted and add some logic to say that serialization is blocked? Here is an example of a test where the issue comes up

I don't think we need this, yeah. If we don't serialize then we don't need it.

I thought we added ToPartial to the other aggs and them down.

Can you clarify what you mean by this comment?

There's a withoutEvaluator method you should be able to call on the TestCase that should do all this. should, though I think we mostly use it for scalars at the moment.

I've tried removing these overrides and adding in withoutEvaluator calls on the test cases but it still throws an exception as we don't actually make use of the canBuildEvaluator that is set by the method in the AbstractAggregationTestCase class (this is the only reference to the value). Is there something else I'm missing here?

I also notice we removed the TestCase.typeError test cases along with the TestCase.getExpectedTypeError() method used here so I believe we have to handle this differently here somehow but I'm not clear what the intended process for this is. Any recommendations?

I think we can modify this to just define a series of test cases including all of the expected valid types but still overriding these underlying test cases. If this sounds like a good plan I seem to be missing a way to provide float test cases (not sure if this is a known gap in MultiRowTestCaseSupplier?) and I'm also unclear if this would mean we want a SparklineErrorTests file as well?

We actually can't implement SparklineErrorTests as it has too many arguments and fails to generate permutations due to this check. I also realize that we don't need to cover float here as our other aggregates don't seem to either so I'll adjust the field types to remove float. We are still left with the above option of building a test case for each permutation of input types but this does create a fairly large number of tests (multiple thousands). Is this something we want or is there a better option here?

This seems wrong? If there is something in outputPages it can produce more data without extra input, right?

Maybe I'm not clear on what this function is meant to do. From the description it sounds like this returns true if the function can generate outputPages without needing new input pages. Since this operator specifically takes in one input page and generates one output page it seems like this should return false? Let me know if my understanding is wrong.

Are those switch statements similar enough in this and the next 3 method to extract a method? Seems repetitive right now.

The switch statements are the same in that they are on the same types but each of the three methods has to do a different series of actions as a result (one retrieves a block, one generates a default value, one calls the underlying toBlock. Ideally we could work on a more generic interface (i.e. instead of LongLongBucketedSort/LongDoubleBucketedSort they all implement a more generic BucketedSort interface with the functions like toBlock or collect) but that doesn't currently exist and seems like a very big refactor that is out of scope. Maybe it's more of a long term goal after this change if we see this being a useful pattern.

Isn't the return type actually an array of field().dataType()?

Correct but DataType does not have array versions of the various types. For other aggregates that return an array (ex. Values) we do the same thing as we're doing here and just return the type of the elements of the array (see here).

Right. there isn't a "this is always an array" type. One day we might have a "this is never an array" marker on types - but when we have it this is a place that'll have to clear that flag.

appendBlock @throws IllegalArgumentException if the given block does not have the same number of positions as the blocks in this Page
Is this a leak if outputPage.appendBlock(passthroughBlock); throws? We might need a catch and handle it.

I believe this will just throw an exception in the same way that other operators with this call would (example 1 and example 2). I don't see in other instances that we catch and handle this case rather than just letting the exception throw. Is there something different we should be doing in this case?

Nah, unit tests is fine the for error case.

Wants javadoc I think. Sparkline really is a term of art. Maybe a link to wikipedia.

Right. there isn't a "this is always an array" type. One day we might have a "this is never an array" marker on types - but when we have it this is a place that'll have to clear that flag.

Worth fixing in the follow up. It's just to make sure that the error messages you make when you have bad parameters look sane. And that you've covered all valid cases in your SparklineTests.

This is not the right place for this check. All these checks on the restrictions for each function parameter should happen in the function itself. For example, see Top.resolvetTypeLimit().

👍
I hadn't seen this one.