Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

yuchenglin
Copy link

@yuchenglin yuchenglin commented Jan 16, 2025

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

@yuchenglin yuchenglin requested a review from a team as a code owner January 16, 2025 18:41
@yuchenglin
Copy link
Author

@yuchenglin please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

@LucGenetier
Copy link
Contributor

✅ No public API change.

@LucGenetier
Copy link
Contributor

✅ No public API change.

@Shpakh-MSFT Shpakh-MSFT added the Breaking Internals Breaking change - may require a PA Client update. label Jan 17, 2025
@adithyaselv
Copy link
Contributor

adithyaselv commented Jan 17, 2025

can you please add a test like this .

N = DelegableDataSource;

UDF(): DelegableDataSource = N;

N2 = Filter(UDF(), "ColumnName" <> "")

this doesn't work today for the same reasons discussed offline

@yuchenglin
Copy link
Author

can you please add a test like this .

N = DelegableDataSource;

UDF(): DelegableDataSource = N;

N2 = Filter(UDF(), "ColumnName" <> "")

this doesn't work today for the same reasons discussed offline

Synced offline. This can be better testing at Client

@yuchenglin
Copy link
Author

With option 2,

  1. Client tests (code gen) are failing since it will add appendQueryOptionsAsync to original query. From the query itself, the behavior looks isn't changed.
    https://msazure.visualstudio.com/OneAgile/_build/results?buildId=112823118&view=ms.vss-test-web.build-test-results-tab

  2. We got a report from Playground. There are some class out there depends on this interface

System.MissingMethodException: Method not found: 'Boolean Microsoft.PowerFx.Core.Entities.IExternalDataSource.get_IsDelegatable()'.
at Microsoft.PowerFx.Dataverse.DelegationIRVisitor.Visit(ResolvedObjectNode node, Context context)

@yuchenglin yuchenglin force-pushed the yucl/udf_dataSource_delegatable branch from 35611bf to 6d3adac Compare January 22, 2025 02:08
@yuchenglin yuchenglin removed the Breaking Internals Breaking change - may require a PA Client update. label Jan 22, 2025
@LucGenetier
Copy link
Contributor

✅ 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)
Copy link
Contributor

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

Suggested change
if (base.IsServerDelegatable(callNode, binding) || IsDelegatable)
return a || b || c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants