Util.mapOperation does not always apply recurse:true

[rubensworks]

In this piece of code, the result is a different operation than the input operation.
I would have expected that by setting recurse to true, that the returned operation would also be handled.
Instead, I have to manually call the mapping logic on this operation myself.
I assume this is not supposed to be needed?

There are probably some infinite loop concerns to be taken into account here.

[joachimvh]

I would also have expected that :D . Not really sure what the problem is since the code for handling this is not that complex. If recurse is true it should just fall through to the normal handling. And even if the result was not a valid operation at least an error should have been thrown.

Do you have an example of where it fails so I can check more in detail?

[rubensworks]

Not really sure what the problem is since the code for handling this is not that complex.

It looks like that once one callback has been invoked, that the resulting operation is then always handled by the following switch-case.
You probably need a check there that recursively calls mapOperation if the returned operation type does not equal the input operation type.

Do you have an example of where it fails so I can check more in detail?

Here's an example: https://github.com/comunica/comunica/blob/master/packages/bus-query-operation/lib/Bindings.ts#L101-L104
Removing that materializeOperation(...) invocation illustrates the problem.

[joachimvh]

It looks like that once one callback has been invoked, that the resulting operation is then always handled by the following switch-case.
You probably need a check there that recursively calls mapOperation if the returned operation type does not equal the input operation type.

The callbacks object which should contain your custom callbacks is still passed recursively, independent of what the returned operation typeis, since it checks the type of the result, not the input, unless I'm missing something. But I'll have a closer look.

[joachimvh]

Using this input and the following code:

const op = ...;
console.log(AlgebraUtil.mapOperation(op, {
    extend: (op: Algebra.Extend) => {
        return {
            recurse: true,
            result: op.input,
        };
    },
    bgp: (op: Algebra.Bgp) => {
        console.log('recursion!!!');
        return {
            recurse: true,
            result: op,
        };
    },
}));

recursion!!! gets printed, so the callbacks still seem to be called. Is this different from your situation?

[rubensworks]

Yes, it's slightly different. Try adding a map entry for group, I think that will not work anymore.

[joachimvh]

Ah yea, I see the issue now. The callback for group will not be called but it will be called for its parameters. Not sure yet I actually want to fix this now :D . The solution might be uglier than just forcing users to do the call themselves if they change the root object.

[rubensworks]

IMO, a fix is definitely need (and shouldn't be too hard).
However, since users can easily fix it themselves, I don't think this is an urgent thing to fix.

[joachimvh]

It's just that I wouldn't classify it as a bug at this point but an enhancement request. Recursion currently specifically means that it will recurse over the parameters of the result object, but not the result object itself. But I will have a look if there is a clean way to change that. Just don't want to make it uglier for most cases because of a specific edge case.