-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix delegation check on UDF for datasource type #2816
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree company="Microsoft" |
✅ No public API change. |
src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/RecalcEngineTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs
Outdated
Show resolved
Hide resolved
✅ No public API change. |
can you please add a test like this .
this doesn't work today for the same reasons discussed offline |
Synced offline. This can be better testing at Client |
With option 2,
|
35611bf
to
6d3adac
Compare
✅ No public API change. |
@@ -52,7 +52,17 @@ public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding) | |||
Contracts.AssertValue(binding); | |||
Contracts.Assert(binding.GetInfo(callNode).Function is UserDefinedFunction udf && udf.Binding != null); | |||
|
|||
return base.IsServerDelegatable(callNode, binding) || IsDelegatable; | |||
if (base.IsServerDelegatable(callNode, binding) || IsDelegatable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we just have 1 return like
if (base.IsServerDelegatable(callNode, binding) || IsDelegatable) | |
return a || b || c |
Probelm
UDF with direct assigned datasource type such as
UDF1():Accounts = Accounts
is always calculated as non-delegable.There is no issue for
UDF2():Accounts = Filter(Account, ...)
since it would go through override validation in Filter class, where check data source metadata directly. UDF gets IsDelegatable from Top which is Filter().Option 1 (current)
Handle in overridden
IsServerDelegatable
of UserDefinedFunction. It will go check data source metadata directly if it's FirstNameNode and haven't been processed.Option 2
The delegable check on FirstNameNode during binding validates the data is type of
IExternalDelegatableSymbol
, but from this scenario,IExternalCdsDataSource
is not implanted this interface. So always returns IsServerDelegatable false.I decided to add the implantation to align with
IsPagable
.https://github.com/microsoft/Power-Fx/blob/0fa19687742caa771c4a8f0716f07005497bec49/src/libraries/Microsoft.PowerFx.Core/Binding/Binder.cs#L688C13-L688C75
https://dev.azure.com/msazure/OneAgile/_workitems/edit/29998102