feature: Allow custom window functions to be registered with the driver #1220
Conversation
|
Hi @mattn, how can I help in getting this merged? |
|
Sorry my delay. I'll look into it in later. |
|
Look good for me. @rittneje any opinion? |
|
A friendly bump on this @rittneje |
|
@mattn This would be very helpful to get in for the users of https://github.com/goccy/bigquery-emulator I cannot merge my feature branch into the project as it uses a forked Is there anything else I could do to push this along? |
|
Yes, I'm thinking this is useful but I want to wait @rittneje 's opinion. |
|
Following up again! |
|
@rittneje Did you have any thoughts on this small PR? |
|
@ohaibbq Sorry, I really don't have the time for an in-depth review of this. (And I wouldn't describe this as a "small PR".) |
| case reflect.Ptr: | ||
| implReturnsPointer = true | ||
| case reflect.Interface: | ||
| implReturnsPointer = false |
There was a problem hiding this comment.
- How can you conclude whether it returns a pointer or not in this case?
- Saying it doesn't return a pointer and allowing that seems to contradict the error message in the default case.
There was a problem hiding this comment.
You're right that I can't conclude if it returns a pointer or not in this case.
It does contradict the error message in the default case, but it was previously allowed in the initial implementation of user-defined functions #229.
Perhaps there is a better name for this variable.
| return errors.New("SQlite aggregator doesn't have a Step() function") | ||
| return errors.New("SQLite aggregator doesn't have a Step() function") | ||
| } | ||
| err := ai.setupStepInterface(stepFn, &ai.stepArgConverters, &ai.stepVariadicConverter, implReturnsPointer, "Step()") |
There was a problem hiding this comment.
I don't entirely understand this call. The penultimate parameter to setupStepInterface is named isImplPointer, which actually means there is a method receiver? And that doesn't seem to have anything to do with implReturnsPointer.
There was a problem hiding this comment.
I could rename the parameter to hasMethodReceiver.
I maintained the behavior from the original implementation, where we'd only skip the method receiver if the agg.Kind() == reflect.Pointer, but if we must always return a pointer, then there should always be a method receiver.
According to the reflect.Type docs:
// For a non-interface type T or *T, the returned Method's Type and Func
// fields describe a function whose first argument is the receiver,
// and only exported methods are accessible.
When running sqlite3_test.go, I did not encounter any cases where implReturnsPointer would be false. I wonder if we should remove this branch of the logic.
|
|
||
| //export inverseTrampoline | ||
| func inverseTrampoline(ctx *C.sqlite3_context, argc C.int, argv **C.sqlite3_value) { | ||
| args := (*[(math.MaxInt32 - 1) / unsafe.Sizeof((*C.sqlite3_value)(nil))]*C.sqlite3_value)(unsafe.Pointer(argv))[:int(argc):int(argc)] |
There was a problem hiding this comment.
Can you explain what this is doing?
There was a problem hiding this comment.
I can try my best. For the step and inverse interfaces, SQLite passes us argc, the number of arguments to consume, and argv a double pointer to underlying sqlite3_values. A simple explanation of this code is that it initializes a slice of C.sqlite3_values with the length of argc.
In the initial implementation, we used the value
1 << 30 to represent the maximum possible length of this array.
#238 identified that this caused an overflow error. It was changed to use the maximum int32 size, divided by the size of nil *C.sqlite3_value, to limit its length without overflowing.
The crux seems to be that we cannot dynamically initialize an array with a length equal to argc. We must specify a constant size at compile time.
Once this array is initialized, we slice it down to the correct length as determined by argc.
I'm not familiar enough with Go / C to be sure of the exact performance implications of this, but
if you examine the pointer list prior to slicing, you can see that it contains 268,435,455 ((math.MaxInt31 - 1) / 8) elements. it seems like it would be much better if there were a way to only initialize it to argc.
|
@rittneje I appreciate the time you've spent on your initial review of this. Perhaps I can clean up the confusing interface of allowing implementations that return When testing registering an implementation that returns If you don't have much time to continue review, maybe we can defer to @mattn's review since you've gotten the chance to gloss over the code. |
|
@mattn any updates on this? |
This implements the registration of user-defined window functions. It extends the existing
RegisterAggregatorinterface to allow theimplto implementInverseandValue, in which case, the aggregator can be used both as a regular aggregate function and a window function.Closes #1215