Skip to content

Commit

Permalink
PR feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
@anderson-joyle
anderson-joyle committed Feb 6, 2025
1 parent 6f1a3a6 commit 4a6b07c
Show file tree
Hide file tree
Showing 14 changed files with 243 additions and 187 deletions.
17 changes: 17 additions & 0 deletions src/libraries/Microsoft.PowerFx.Core/Functions/Utils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,14 @@
// Licensed under the MIT license.

using System.Globalization;
using System.Linq;
using System.Numerics;
using Microsoft.PowerFx.Core.IR;
using Microsoft.PowerFx.Core.IR.Nodes;
using Microsoft.PowerFx.Core.Localization;
using Microsoft.PowerFx.Core.Types;
using Microsoft.PowerFx.Core.Utils;
using Microsoft.PowerFx.Types;

namespace Microsoft.PowerFx.Core.Functions
{
Expand All @@ -23,5 +27,18 @@ public static string GetLocalizedName(this FunctionCategories category, CultureI
{
return StringResources.Get(category.ToString(), culture.Name);
}

public static void FunctionSupportColumnNamesAsIdentifiersDependencyUtil(this CallNode node, DependencyVisitor visitor)
{
var aggregateType0 = node.Args[0].IRContext.ResultType as AggregateType;

foreach (TextLiteralNode arg in node.Args.Skip(1).Where(a => a is TextLiteralNode))
{
if (aggregateType0.TryGetFieldType(arg.LiteralValue, out _))
{
visitor.AddDependency(aggregateType0.TableSymbolName, arg.LiteralValue);
}
}
}
}
}
9 changes: 7 additions & 2 deletions src/libraries/Microsoft.PowerFx.Core/IR/IRTranslator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,13 @@
namespace Microsoft.PowerFx.Core.IR
{
internal class IRResult
{
public IntermediateNode TopNode;
{
// IR top node after transformations.
public IntermediateNode TopNode;

// Original IR node, without transformations.
public IntermediateNode TopOriginalNode;

public ScopeSymbol RuleScopeSymbol;
}

Expand Down
115 changes: 27 additions & 88 deletions src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
using System.Text;
using Microsoft.PowerFx.Core.IR.Nodes;
using Microsoft.PowerFx.Core.IR.Symbols;
using Microsoft.PowerFx.Core.Types;
using Microsoft.PowerFx.Core.Utils;
using Microsoft.PowerFx.Types;
using static Microsoft.PowerFx.Syntax.PrettyPrintVisitor;

Expand All @@ -14,7 +16,7 @@ namespace Microsoft.PowerFx.Core.IR
// IR has already:
// - resolved everything to logical names.
// - resolved implicit ThisRecord
internal class DependencyVisitor : IRNodeVisitor<DependencyVisitor.RetVal, DependencyVisitor.DependencyContext>
internal sealed class DependencyVisitor : IRNodeVisitor<DependencyVisitor.RetVal, DependencyVisitor.DependencyContext>
{
// Track reults.
public DependencyInfo Info { get; private set; } = new DependencyInfo();
Expand Down Expand Up @@ -50,13 +52,10 @@ public override RetVal Visit(ColorLiteralNode node, DependencyContext context)

public override RetVal Visit(RecordNode node, DependencyContext context)
{
// Read all the fields. The context will determine if the record is referencing a data source
// Visit all field values in case there are CallNodes. Field keys should be handled by the function caller.
foreach (var kv in node.Fields)
{
AddField(context, context.ScopeType?.TableSymbolName, kv.Key.Value);

// Reset the context for field values in case we are operating in a write context.
kv.Value.Accept(this, new DependencyContext());
kv.Value.Accept(this, context);
}

return null;
Expand Down Expand Up @@ -109,20 +108,16 @@ public override RetVal Visit(ScopeAccessNode node, DependencyContext context)
if (_scopeTypes.TryGetValue(sym.Parent.Id, out var type))
{
// Ignore ThisRecord scopeaccess node. e.g. Summarize(table, f1, Sum(ThisGroup, f2)) where ThisGroup should be ignored.
if (type is TableType tableType && node.IRContext.ResultType is not AggregateType)
if (type is TableType tableType && tableType.TryGetFieldType(sym.Name.Value, out _))
{
var tableLogicalName = tableType.TableSymbolName;
var fieldLogicalName = sym.Name.Value;

AddField(context, tableLogicalName, fieldLogicalName);
AddDependency(tableType.TableSymbolName, sym.Name.Value);

return null;
}
}
}

// Any symbol access here is some temporary local, and not a field.
return null;

return null;
}

// field // IR will implicity recognize as ThisRecod.field
Expand All @@ -141,7 +136,7 @@ public override RetVal Visit(RecordFieldAccessNode node, DependencyContext conte
if (tableLogicalName != null)
{
var fieldLogicalName = node.Field.Value;
AddField(context, tableLogicalName, fieldLogicalName);
AddDependency(tableLogicalName, fieldLogicalName);
}
}

Expand All @@ -152,24 +147,15 @@ public override RetVal Visit(ResolvedObjectNode node, DependencyContext context)
{
if (node.IRContext.ResultType is AggregateType aggregateType)
{
var tableLogicalName = aggregateType.TableSymbolName;
if (context.WriteState)
{
tableLogicalName = context.ScopeType?.TableSymbolName;
}

if (tableLogicalName != null)
{
AddField(context, tableLogicalName, null);
}
AddDependency(aggregateType.TableSymbolName, null);
}

CheckResolvedObjectNodeValue(node, context);

return null;
}

protected virtual void CheckResolvedObjectNodeValue(ResolvedObjectNode node, DependencyContext context)
public void CheckResolvedObjectNodeValue(ResolvedObjectNode node, DependencyContext context)
{
if (node.Value is NameSymbol sym)
{
Expand All @@ -181,14 +167,14 @@ protected virtual void CheckResolvedObjectNodeValue(ResolvedObjectNode node, Dep
if (symTable.IsThisRecord(sym))
{
// "ThisRecord". Whole entity
AddField(context, tableLogicalName, null);
AddDependency(type.TableSymbolName, null);
return;
}

// on current table
var fieldLogicalName = sym.Name;

AddField(context, tableLogicalName, fieldLogicalName);
AddDependency(type.TableSymbolName, fieldLogicalName);
}
}
}
Expand Down Expand Up @@ -224,73 +210,27 @@ public class RetVal

public class DependencyContext
{
// Indicates the scanning is working on the args of a non self-contained function.
public bool WriteState { get; init; }

// If WriteState is true, this will be set with the arg0 type.
public TableType ScopeType { get; init; }

public DependencyContext()
{
}
}

// Translate relationship names to actual field references.
public virtual string Translate(string tableLogicalName, string fieldLogicalName)
{
return fieldLogicalName;
}

// if fieldLogicalName, then we're taking a dependency on entire record.
private void AddField(Dictionary<string, HashSet<string>> list, string tableLogicalName, string fieldLogicalName)
public void AddDependency(string tableLogicalName, string fieldLogicalName)
{
if (tableLogicalName == null)
{
return;
}

if (!list.TryGetValue(tableLogicalName, out var fieldReads))
if (!Info.Dependencies.ContainsKey(tableLogicalName))
{
fieldReads = new HashSet<string>();
list[tableLogicalName] = fieldReads;
Info.Dependencies[tableLogicalName] = new HashSet<string>();
}

if (fieldLogicalName != null)
{
var name = Translate(tableLogicalName, fieldLogicalName);
fieldReads.Add(name);
}
}

public void AddFieldRead(string tableLogicalName, string fieldLogicalName)
{
if (Info.FieldReads == null)
{
Info.FieldReads = new Dictionary<string, HashSet<string>>();
}

AddField(Info.FieldReads, tableLogicalName, fieldLogicalName);
}

public void AddFieldWrite(string tableLogicalName, string fieldLogicalName)
{
if (Info.FieldWrites == null)
{
Info.FieldWrites = new Dictionary<string, HashSet<string>>();
}

AddField(Info.FieldWrites, tableLogicalName, fieldLogicalName);
}

public void AddField(DependencyContext context, string tableLogicalName, string fieldLogicalName)
{
if (context.WriteState)
{
AddFieldWrite(tableLogicalName, fieldLogicalName);
}
else
if (fieldLogicalName != null)
{
AddFieldRead(tableLogicalName, fieldLogicalName);
Info.Dependencies[tableLogicalName].Add(fieldLogicalName);
}
}
}
Expand All @@ -310,29 +250,28 @@ public class DependencyInfo
/// The formula "Name & 'Primary Contact'.'Full Name' & Sum(Contacts, 'Number Of Childeren')" would return
/// "contact" => { "fullname", "numberofchildren" }.
/// </example>
public Dictionary<string, HashSet<string>> FieldReads { get; set; }
#pragma warning restore CS1570 // XML comment has badly formed XML

public Dictionary<string, HashSet<string>> FieldWrites { get; set; }
public Dictionary<string, HashSet<string>> Dependencies { get; set; }

public bool HasWrites => FieldWrites != null && FieldWrites.Count > 0;
public DependencyInfo()
{
Dependencies = new Dictionary<string, HashSet<string>>();
}

public override string ToString()
{
StringBuilder sb = new StringBuilder();
DumpHelper(sb, "Read", FieldReads);
DumpHelper(sb, "Write", FieldWrites);
DumpHelper(sb, Dependencies);

return sb.ToString();
}

private static void DumpHelper(StringBuilder sb, string kind, Dictionary<string, HashSet<string>> dict)
private static void DumpHelper(StringBuilder sb, Dictionary<string, HashSet<string>> dict)
{
if (dict != null)
{
foreach (var kv in dict)
{
sb.Append(kind);
sb.Append("Entity");
sb.Append(" ");
sb.Append(kv.Key);
sb.Append(": ");
Expand Down
16 changes: 11 additions & 5 deletions src/libraries/Microsoft.PowerFx.Core/Public/CheckResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.CodeAnalysis;
using Microsoft.PowerFx.Core.Binding;
using Microsoft.PowerFx.Core.IR;
using Microsoft.PowerFx.Core.IR.Nodes;
using Microsoft.PowerFx.Core.Localization;
using Microsoft.PowerFx.Core.Logging;
using Microsoft.PowerFx.Core.Public;
Expand Down Expand Up @@ -483,14 +484,16 @@ private void VerifyReturnTypeMatch()
/// <summary>
/// Compute the dependencies. Called after binding.
/// </summary>
[Obsolete("Preview")]
public DependencyInfo ApplyDependencyInfoScan()
{
var ir = ApplyIR(); //throws on errors

var ctx = new DependencyVisitor.DependencyContext();
var visitor = new DependencyVisitor();

ir.TopNode.Accept(visitor, ctx);
// Using the original node without transformations. This simplifies the dependency analysis for PFx.DV side.
ir.TopOriginalNode.Accept(visitor, ctx);

return visitor.Info;
}
Expand Down Expand Up @@ -549,17 +552,19 @@ internal IRResult ApplyIR()
{
if (_irresult == null)
{
IntermediateNode irnode = null;

// IR should not create any new errors.
var binding = this.ApplyBindingInternal();
this.ThrowOnErrors();
(var irnode, var ruleScopeSymbol) = IRTranslator.Translate(binding);
(var originalIRNode, var ruleScopeSymbol) = IRTranslator.Translate(binding);

var list = _engine.IRTransformList;
if (list != null)
{
foreach (var transform in list)
{
irnode = transform.Transform(irnode, _errors);
irnode = transform.Transform(irnode ?? originalIRNode, _errors);

// Additional errors from phases.
// Stop any further processing if we have errors.
Expand All @@ -569,7 +574,8 @@ internal IRResult ApplyIR()

_irresult = new IRResult
{
TopNode = irnode,
TopNode = irnode ?? originalIRNode,
TopOriginalNode = originalIRNode,
RuleScopeSymbol = ruleScopeSymbol
};
}
Expand Down Expand Up @@ -606,7 +612,7 @@ public FormulaType GetNodeType(TexlNode node)
/// </summary>
/// <param name="node"></param>
/// <returns>Null if the node is not bound.</returns>
public FunctionInfo GetFunctionInfo(CallNode node)
public FunctionInfo GetFunctionInfo(Syntax.CallNode node)
{
if (node == null)
{
Expand Down
26 changes: 19 additions & 7 deletions src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -469,15 +469,27 @@ protected static List<IntermediateNode> CreateIRCallNodeCollect(CallNode node, I

public override bool ComposeDependencyInfo(IRCallNode node, DependencyVisitor visitor, DependencyVisitor.DependencyContext context)
{
var newContext = new DependencyVisitor.DependencyContext()
{
WriteState = true,
ScopeType = node.Args[0].IRContext.ResultType as TableType
};
var tableType = (TableType)node.Args[0].IRContext.ResultType;

foreach (var arg in node.Args.Skip(1))
{
arg.Accept(visitor, newContext);
var argType = arg.IRContext.ResultType;
string argTableSymbolName = null;

if (argType is AggregateType aggregateType)
{
// If argN is a record/table and has a table symbol name, it means we are referencing another entity.
// We then need to add a dependency to the table symbol name as well.
argTableSymbolName = aggregateType.TableSymbolName;
}

foreach (var name in argType._type.GetAllNames(DPath.Root))
{
visitor.AddDependency(tableType.TableSymbolName, name.Name.Value);
visitor.AddDependency(argTableSymbolName, name.Name.Value);
}

arg.Accept(visitor, context);
}

return true;
Expand Down Expand Up @@ -511,7 +523,7 @@ internal override IntermediateNode CreateIRCallNode(PowerFx.Syntax.CallNode node
public override bool ComposeDependencyInfo(IRCallNode node, DependencyVisitor visitor, DependencyVisitor.DependencyContext context)
{
var tableType = (TableType)node.Args[0].IRContext.ResultType;
visitor.AddFieldWrite(tableType.TableSymbolName, "Value");
visitor.AddDependency(tableType.TableSymbolName, "Value");

return true;
}
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Join.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ public override bool ComposeDependencyInfo(IRCallNode node, DependencyVisitor vi
foreach (var field in recordNode.Fields)
{
var tableType = (TableType)sourceArg.IRContext.ResultType;
visitor.AddField(context, tableType.TableSymbolName, field.Key.Value);
visitor.AddDependency(tableType.TableSymbolName, field.Key.Value);
}

break;
Expand Down
Loading

0 comments on commit 4a6b07c

Please sign in to comment.