Skip to content

Commit

Permalink
Fix delegation check on UDF for datasource type (#2816)
Browse files Browse the repository at this point in the history
## 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 which
haven't been processed. Also, unify the validation to use `IsDelegatable `.

## 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
  • Loading branch information
yuchenglin authored Jan 29, 2025
1 parent 25082cf commit 7b59ddc
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 26 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ namespace Microsoft.PowerFx.Core.Functions
/// This includings the binding (and hence IR for evaluation) -
/// This is conceptually immutable after initialization - if the body or signature changes, you need to create a new instance.
/// </summary>
internal class UserDefinedFunction : TexlFunction, IExternalPageableSymbol, IExternalDelegatableSymbol
internal class UserDefinedFunction : TexlFunction, IExternalPageableSymbol
{
private readonly bool _isImperative;
private readonly IEnumerable<UDFArg> _args;
Expand All @@ -44,15 +44,23 @@ internal class UserDefinedFunction : TexlFunction, IExternalPageableSymbol, IExt

public bool IsPageable => _binding.IsPageable(_binding.Top);

public bool IsDelegatable => _binding.IsDelegatable(_binding.Top);

public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding)
{
Contracts.AssertValue(callNode);
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) || _binding.IsDelegatable(_binding.Top))
{
return true;
}

if (_binding.Top.Kind == NodeKind.FirstName && ArgValidators.DelegatableDataSourceInfoValidator.TryGetValidValue(_binding.Top, _binding, out _))
{
return true;
}

return false;
}

public override bool SupportsParamCoercion => true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,16 @@

using System;
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Text;
using Microsoft.PowerFx.Core.Entities.QueryOptions;
using Microsoft.PowerFx.Core.Errors;
using Microsoft.PowerFx.Core.Functions.Delegation;
using Microsoft.PowerFx.Core.Tests.Helpers;
using Microsoft.PowerFx.Core.Texl;
using Microsoft.PowerFx.Core.Types;
using Microsoft.PowerFx.Core.Utils;
using Microsoft.PowerFx.Types;
using Xunit;

Expand Down Expand Up @@ -73,20 +77,34 @@ public void TestDelegableExpressions_ColumnNamesAsLiteralStrings(string expressi
};

TestDelegableExpressions(features, expression, isDelegable);
}

[Theory]
[InlineData("UDF1()", "UDF1():Accounts = Accounts;", true)]
[InlineData("Filter(UDF1(), \"name\" <> \"\")", "UDF1():Accounts = Accounts;", true)]
public void TestDelegableExpressions_UserDfeinedFunction(string expression, string script, bool isDelegable)
{
TestDelegableExpressions(Features.PowerFxV1, expression, isDelegable, script);
}

private void TestDelegableExpressions(Features features, string expression, bool isDelegable)
private void TestDelegableExpressions(Features features, string expression, bool isDelegable, string udfScript = null)
{
var symbolTable = new DelegatableSymbolTable();
symbolTable.AddEntity(new AccountsEntity());
symbolTable.AddVariable("varString", FormulaType.String);

symbolTable.AddEntity(new AccountsEntity());
symbolTable.AddVariable("varString", FormulaType.String);
symbolTable.AddType(new DName("Accounts"), FormulaType.Build(AccountsTypeHelper.GetDType()));

var config = new PowerFxConfig(features)
{
SymbolTable = symbolTable
};

var engine = new Engine(config);
};

var engine = new Engine(config);
if (!string.IsNullOrWhiteSpace(udfScript))
{
engine.AddUserDefinedFunction(udfScript, CultureInfo.InvariantCulture);
}

var result = engine.Check(expression);
Assert.True(result.IsSuccess);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Globalization;
using System.Linq;
using System.Linq.Expressions;
using System.Text;
using System.Threading;
using System.Threading.Tasks;
Expand Down Expand Up @@ -796,26 +797,39 @@ public void DelegableUDFTest()
var recalcEngine = new RecalcEngine(config);

recalcEngine.AddUserDefinedFunction("A():MyDataSourceTableType = Filter(MyDataSource, Value > 10);C():MyDataSourceTableType = A(); B():MyDataSourceTableType = Filter(C(), Value > 11); D():MyDataSourceTableType = { Filter(B(), Value > 12); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true);
var func = recalcEngine.Functions.WithName("A").First() as UserDefinedFunction;

Assert.True(func.IsAsync);
Assert.True(func.IsDelegatable);

func = recalcEngine.Functions.WithName("C").First() as UserDefinedFunction;

Assert.True(func.IsAsync);
Assert.True(func.IsDelegatable);
var result = recalcEngine.Check("A()");
Assert.True(result.IsSuccess);
var callNode = result.Binding.Top.AsCall();
Assert.NotNull(callNode);
var callInfo = result.Binding.GetInfo(callNode);
Assert.True(callInfo.Function.IsAsyncInvocation(callNode, result.Binding));
Assert.True(callInfo.Function.IsServerDelegatable(callNode, result.Binding));

func = recalcEngine.Functions.WithName("B").First() as UserDefinedFunction;
result = recalcEngine.Check("B()");
Assert.True(result.IsSuccess);
callNode = result.Binding.Top.AsCall();
Assert.NotNull(callNode);
callInfo = result.Binding.GetInfo(callNode);
Assert.True(callInfo.Function.IsAsyncInvocation(callNode, result.Binding));
Assert.True(callInfo.Function.IsServerDelegatable(callNode, result.Binding));

Assert.True(func.IsAsync);
Assert.True(func.IsDelegatable);
result = recalcEngine.Check("C()");
Assert.True(result.IsSuccess);
callNode = result.Binding.Top.AsCall();
Assert.NotNull(callNode);
callInfo = result.Binding.GetInfo(callNode);
Assert.True(callInfo.Function.IsAsyncInvocation(callNode, result.Binding));
Assert.True(callInfo.Function.IsServerDelegatable(callNode, result.Binding));

func = recalcEngine.Functions.WithName("D").First() as UserDefinedFunction;
result = recalcEngine.Check("D()");
Assert.False(result.IsSuccess);
callNode = result.Binding.Top.AsCall();
Assert.NotNull(callNode);
callInfo = result.Binding.GetInfo(callNode);

// Imperative function is not delegable
Assert.True(func.IsAsync);
Assert.True(!func.IsDelegatable);
Assert.True(callInfo.Function.IsAsyncInvocation(callNode, result.Binding));
Assert.False(callInfo.Function.IsServerDelegatable(callNode, result.Binding));

// Binding fails for recursive definitions and hence function is not added.
Assert.False(recalcEngine.AddUserDefinedFunction("E():Void = { E(); };", CultureInfo.InvariantCulture, symbolTable: recalcEngine.EngineSymbols, allowSideEffects: true).IsSuccess);
Expand Down

0 comments on commit 7b59ddc

Please sign in to comment.