-
Notifications
You must be signed in to change notification settings - Fork 334
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. |
src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs
Outdated
Show resolved
Hide resolved
is it possible to just get rid of this property and directly call _binding.IsDelegatable(_binding.Top) below . this pattern makes sense for this pattern may not be needed here at all . can you investigate ? Refers to: src/libraries/Microsoft.PowerFx.Core/Functions/UserDefinedFunction.cs:47 in 6d3adac. [](commit_id = 6d3adac, deletion_comment = False) |
Looks like |
✅ No public API change. |
…l/udf_dataSource_delegatable
d256a03
to
f865a25
Compare
✅ No public API change. |
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