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

Update logic to determine whether a LookUp call with a reduction formula argument can be delegated #2065

Merged
merged 13 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ public sealed class Features
/// </summary>
internal bool JsonFunctionAcceptsLazyTypes { get; set; }

/// <summary>
/// Enables more robust lookup reduction delegation.
/// </summary>
internal bool IsLookUpReductionDelegationEnabled { get; set; }

/// <summary>
/// This is specific for Cards team and it is a temporary feature.
/// It will be soon deleted.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ public override bool TryGetDelegationMetadata(CallNode node, TexlBinding binding
return true;
}

protected bool IsValidDelegatableFilterPredicateNode(TexlNode dsNode, TexlBinding binding, FilterOpMetadata filterMetadata, bool generateHints = true)
protected bool IsValidDelegatableFilterPredicateNode(TexlNode dsNode, TexlBinding binding, FilterOpMetadata filterMetadata, bool generateHints = true, bool enforceBoolean = true)
ian-legler marked this conversation as resolved.
Show resolved Hide resolved
{
Contracts.AssertValue(dsNode);
Contracts.AssertValue(binding);
Expand Down Expand Up @@ -105,7 +105,7 @@ protected bool IsValidDelegatableFilterPredicateNode(TexlNode dsNode, TexlBindin

case NodeKind.FirstName:
{
if (!IsNodeBooleanOptionSetorBooleanFieldorView(dsNode, binding))
if (enforceBoolean && !IsNodeBooleanOptionSetorBooleanFieldorView(dsNode, binding))
{
SuggestDelegationHint(dsNode, binding);
return false;
Expand All @@ -121,7 +121,7 @@ protected bool IsValidDelegatableFilterPredicateNode(TexlNode dsNode, TexlBindin

case NodeKind.DottedName:
{
if (!IsNodeBooleanOptionSetorBooleanFieldorView(dsNode, binding))
if (enforceBoolean && !IsNodeBooleanOptionSetorBooleanFieldorView(dsNode, binding))
{
SuggestDelegationHint(dsNode, binding);
return false;
Expand Down Expand Up @@ -162,12 +162,13 @@ protected bool IsValidDelegatableFilterPredicateNode(TexlNode dsNode, TexlBindin

default:
{
if (kind != NodeKind.BoolLit)
if (enforceBoolean && kind != NodeKind.BoolLit)
{
SuggestDelegationHint(dsNode, binding, string.Format(CultureInfo.InvariantCulture, "Not supported node {0}.", kind));
return false;
}

// TODO: if boolean results are not being enforced, are there any other node types that should be considered non-delegable?
break;
}
}
Expand Down
80 changes: 68 additions & 12 deletions src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Lookup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@
// Licensed under the MIT license.

using System.Collections.Generic;
using System.Globalization;
ian-legler marked this conversation as resolved.
Show resolved Hide resolved
using Microsoft.PowerFx.Core.App.ErrorContainers;
using Microsoft.PowerFx.Core.Binding;
using Microsoft.PowerFx.Core.Entities;
using Microsoft.PowerFx.Core.Errors;
using Microsoft.PowerFx.Core.Functions;
using Microsoft.PowerFx.Core.Functions.Delegation;
using Microsoft.PowerFx.Core.Functions.Delegation.DelegationMetadata;
using Microsoft.PowerFx.Core.Functions.Delegation.DelegationStrategies;
using Microsoft.PowerFx.Core.Localization;
using Microsoft.PowerFx.Core.Types;
using Microsoft.PowerFx.Core.Utils;
Expand Down Expand Up @@ -80,7 +82,32 @@ public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding)
return false;
}

FilterOpMetadata metadata = null;
if (!TryGetFilterOpDelegationMetadata(callNode, binding, out var metadata))
{
return false;
}

var args = callNode.Args.Children.VerifyValue();

// without lookup reduction delegation, follow the legacy logic to determine if the function call is delegatable
ian-legler marked this conversation as resolved.
Show resolved Hide resolved
// NOTE that with the reduction delegation enabled, the reduction node can only make a LookUp not delegatable if it is used inside another filter
if (!binding.Features.IsLookUpReductionDelegationEnabled && args.Count > 2 && binding.IsDelegatable(args[2]))
{
SuggestDelegationHint(args[2], binding);
return false;
}

if (args.Count < 2)
{
return false;
}

return IsValidDelegatableFilterPredicateNode(args[1], binding, metadata);
}

private bool TryGetFilterOpDelegationMetadata(CallNode callNode, TexlBinding binding, out FilterOpMetadata metadata)
{
metadata = null;
if (TryGetEntityMetadata(callNode, binding, out IDelegationMetadata delegationMetadata))
{
if (!TryGetValidDataSourceForDelegation(callNode, binding, DelegationCapability.ArrayLookup, out _))
Expand All @@ -101,25 +128,54 @@ public override bool IsServerDelegatable(CallNode callNode, TexlBinding binding)
metadata = dataSource.DelegationMetadata.FilterDelegationMetadata;
}

var args = callNode.Args.Children.VerifyValue();
if (args.Count > 2 && binding.IsDelegatable(args[2]))
{
SuggestDelegationHint(args[2], binding);
return false;
}
return true;
}

if (args.Count < 2)
public override bool IsEcsExcemptedLambda(int index)
ian-legler marked this conversation as resolved.
Show resolved Hide resolved
{
// Only the second argument for lookup is an ECS excempted lambda
return index == 1;
}

public override ICallNodeDelegatableNodeValidationStrategy GetCallNodeDelegationStrategy()
{
return new LookUpCallNodeDelegationStrategy(this);
}

public bool IsValidDelegatableReductionNode(CallNode callNode, TexlNode reductionNode, TexlBinding binding)
{
if (!TryGetFilterOpDelegationMetadata(callNode, binding, out var metadata))
{
return false;
}

return IsValidDelegatableFilterPredicateNode(args[1], binding, metadata);
// use a variation of the filter predicate logic to determine if the reduction formula is delegatable, without enforcing the return type must be boolean
return IsValidDelegatableFilterPredicateNode(reductionNode, binding, metadata, generateHints: false, enforceBoolean: false);
}
ian-legler marked this conversation as resolved.
Show resolved Hide resolved
}

public override bool IsEcsExcemptedLambda(int index)
internal sealed class LookUpCallNodeDelegationStrategy : DelegationValidationStrategy
{
public LookUpCallNodeDelegationStrategy(TexlFunction function)
: base(function)
{
// Only the second argument for lookup is an ECS excempted lambda
return index == 1;
}

public override bool IsValidCallNode(CallNode node, TexlBinding binding, OperationCapabilityMetadata metadata, TexlFunction trackingFunction = null)
{
var function = binding.GetInfo(node)?.Function;
var args = node.Args.Children.VerifyValue();

// if enabled, only for Lookup functions, verify if the reduction formula is valid for delegation
if (binding.Features.IsLookUpReductionDelegationEnabled &&
function is LookUpFunction lookup && args.Count > 2 &&
!lookup.IsValidDelegatableReductionNode(node, args[2], binding))
{
this.SuggestDelegationHint(args[2], binding);
return false;
}

return IsValidCallNodeInternal(node, binding, metadata, trackingFunction ?? Function);
}
}
}