Moving dependency scan to CORE #2827

anderson-joyle · 2025-01-22T22:03:09Z

MikeStall · 2025-01-22T23:47:12Z

src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs

+        /// 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; }


string

I think we will ultimately want to move away from strings.and to something more structured.

MikeStall · 2025-01-22T23:49:41Z

src/libraries/Microsoft.PowerFx.Core/Public/CheckResult.cs

@@ -483,7 +483,27 @@ private void VerifyReturnTypeMatch()
        /// <summary>
        /// Compute the dependencies. Called after binding. 
        /// </summary>
-        public void ApplyDependencyAnalysis()


ApplyDependencyAnalysis

nit - this is breaking.
Maybe keep old one, add [Obsolete] and point it to new one?
#Resolved

Yes, I'll make the change.

MikeStall · 2025-01-22T23:50:16Z

src/libraries/Microsoft.PowerFx.Core/Public/CheckResult.cs

+            if (!IsSuccess)
+            {
+                return null;
+            }


nit - needed? Let ApplyIR() handle this. #Resolved

MikeStall · 2025-01-22T23:52:06Z

src/libraries/Microsoft.PowerFx.Core/Texl/Builtins/Collect.cs

@@ -19,7 +19,8 @@
 using Microsoft.PowerFx.Syntax;
 using Microsoft.PowerFx.Types;
 using CallNode = Microsoft.PowerFx.Syntax.CallNode;
-using RecordNode = Microsoft.PowerFx.Core.IR.Nodes.RecordNode;
+using IRCallNode = Microsoft.PowerFx.Core.IR.Nodes.CallNode;


IRCallNode

I like the IR prefix to avoid collisions. we should follow that convention more often.
#Resolved

LucGenetier · 2025-01-23T04:07:00Z

✅ No public API change. #Resolved

MikeStall · 2025-01-23T05:26:00Z

src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs

+            // The return value is used by DepedencyScanFunctionTests test case.
+            // Returning false to indicate that the function runs a basic dependency scan.
+            // Other functions can override this method to return true if they have a custom dependency scan.
+            return false;


What do these semantics mean? Ie, what is a caller supposed to do differently if we return true vs. false?
#Resolved

Discussed internally.

MikeStall · 2025-01-23T05:26:45Z

src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs

+                    }
+                }
+                else
+                {


Why do we need this logic? #Resolved

MikeStall · 2025-01-23T05:32:26Z

src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs

+
+        public class DependencyContext
+        {
+            public bool WriteState { get; set; }


bool

can we comment what this fields mean? #Resolved

MikeStall · 2025-01-23T05:41:52Z

src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs

+
+            if (fieldLogicalName != null)
+            {
+                var name = Translate(tableLogicalName, fieldLogicalName);


name

used? #Resolved

Yes, from DV side.

MikeStall · 2025-01-23T05:42:01Z

src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs

+        public string Translate(string tableLogicalName, string fieldLogicalName)
+        {
+            return fieldLogicalName;
+        }


needed? #Resolved

Yes, from DV side.

ah, it's virtual now ;)

MikeStall · 2025-01-23T05:49:05Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/DependencyTests.cs

+        [InlineData("Min(local,numberofemployees)", "Read Accounts: numberofemployees;")]
+        [InlineData("Average(local,numberofemployees)", "Read Accounts: numberofemployees;")]
+
+        // Patch


Test:
Patch(local, First(local), { field1 : First(local).field2 } )

local.field1 is a write.
local.field2 is a read

#Resolved

Test case added.

MikeStall · 2025-01-23T05:49:44Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/DependencyTests.cs

+            var functions = new List<TexlFunction>();
+            functions.AddRange(BuiltinFunctionsCore.BuiltinFunctionsLibrary);
+
+            var exceptionList = new HashSet<string>()


What is the rule what is on this list? #Resolved

+1, is this planed for future?

Comment added.

Merging main branch.

MikeStall · 2025-01-24T20:57:32Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/DependencyTests.cs

+        }
+
+        // Some functions might require an different dependecy scan. This test case is to ensure that any new functions that
+        // is not self-contained or has a scope info has been assessed and either added to the exception list or has a dependency scan.


or has a dependency scan.

"overrides <see = TexlFunction.ComposeDependencyInfo> #Resolved

MikeStall · 2025-01-24T21:01:27Z

src/libraries/Microsoft.PowerFx.Core/IR/Visitors/DependencyVisitor.cs

+
+        public class DependencyContext
+        {
+            public bool WriteState { get; set; }


set

can these be 'init' instead of 'set'? To emphasize immutability. #Resolved

jas-valgotar · 2025-01-24T21:14:48Z

src/libraries/Microsoft.PowerFx.Core/Functions/TexlFunction.cs

+                    }
+                    else
+                    {
+                        continue;


continue;

why do we not need to visit this node?
I.e. something like ShowColumns(), can have TextLiteralNode which are column name being used, Are we skipping those? #Resolved

jas-valgotar · 2025-01-24T21:22:05Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/DependencyTests.cs

+        // Set
+        [InlineData("Set(numberofemployees, 200)", "Write Accounts: numberofemployees;")]
+        [InlineData("Set('Address 1: City', 'Account Name')", "Read Accounts: name; Write Accounts: address1_city;")]
+        [InlineData("Set('Address 1: City', 'Address 1: City' & \"test\")", "Read Accounts: address1_city; Write Accounts: address1_city;")]


[InlineData("Set('Address 1: City', 'Address 1: City' & "test")", "Read Accounts: address1_city; Write Accounts: address1_city;")]

Can we add atleast 1 test for all functions that uses Table?

ShowColumns(), RenameColumns(), AddColumn(), DropColumns(), Remove() etc...

jas-valgotar · 2025-01-24T21:26:38Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/DependencyTests.cs

+        // Some functions might require an different dependecy scan. This test case is to ensure that any new functions that
+        // is not self-contained or has a scope info has been assessed and either added to the exception list or has a dependency scan.
+        [Fact]
+        public void DepedencyScanFunctionTests()


DepedencyScanFunctionTests

nice test! #Resolved

LucGenetier · 2025-01-24T22:40:20Z

✅ No public API change.

MikeStall · 2025-01-25T00:31:19Z

src/libraries/Microsoft.PowerFx.Core/Public/CheckResult.cs

+        /// <summary>
+        /// Compute the dependencies. Called after binding. 
+        /// </summary>
+        public DependencyInfo ApplyDependencyInfoScan()


ApplyDependencyInfoScan

Can we [Obsolete("Preview"] this. I think we'll want to make some changes. Like moving away from strings.

Draft

dc7d11b

anderson-joyle requested a review from a team as a code owner January 22, 2025 22:03

MikeStall reviewed Jan 22, 2025

View reviewed changes

anderson-joyle added 2 commits January 22, 2025 21:37

Suppressing comment format.

878fcbf

Fix.

45d7153

MikeStall reviewed Jan 23, 2025

View reviewed changes

anderson-joyle added 2 commits January 23, 2025 09:58

Fixing record visitor.

ca52faf

Merge branch 'main' into andersonf/dependency-scan

8621e2d

Merging main branch.

MikeStall reviewed Jan 24, 2025

View reviewed changes

jas-valgotar reviewed Jan 24, 2025

View reviewed changes

PR feedback.

9cf7085

MikeStall reviewed Jan 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving dependency scan to CORE #2827

Moving dependency scan to CORE #2827

anderson-joyle commented Jan 22, 2025

MikeStall Jan 22, 2025

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 24, 2025

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

anderson-joyle Jan 24, 2025

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

jas-valgotar Jan 24, 2025

anderson-joyle Jan 24, 2025

MikeStall Jan 24, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 24, 2025 •

edited by anderson-joyle

Loading

jas-valgotar Jan 24, 2025 •

edited by anderson-joyle

Loading

jas-valgotar Jan 24, 2025

jas-valgotar Jan 24, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 24, 2025

MikeStall Jan 25, 2025

Moving dependency scan to CORE #2827

Are you sure you want to change the base?

Moving dependency scan to CORE #2827

Conversation

anderson-joyle commented Jan 22, 2025

Choose a reason for hiding this comment

MikeStall Jan 22, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 22, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

MikeStall Jan 22, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

LucGenetier commented Jan 23, 2025 • edited by anderson-joyle Loading

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 23, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeStall Jan 24, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

MikeStall Jan 24, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

jas-valgotar Jan 24, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jas-valgotar Jan 24, 2025 • edited by anderson-joyle Loading

Choose a reason for hiding this comment

LucGenetier commented Jan 24, 2025

Choose a reason for hiding this comment

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 22, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 23, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 24, 2025 •

edited by anderson-joyle

Loading

MikeStall Jan 24, 2025 •

edited by anderson-joyle

Loading

jas-valgotar Jan 24, 2025 •

edited by anderson-joyle

Loading

jas-valgotar Jan 24, 2025 •

edited by anderson-joyle

Loading