Unifying Remove function with PA #2776

anderson-joyle · 2024-12-13T19:42:47Z

Issue #2808.

LucGenetier · 2024-12-13T20:11:29Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-16T17:24:19Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-17T20:08:43Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-19T12:39:46Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-19T16:54:17Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-19T17:52:59Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-26T21:19:17Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-28T01:16:27Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-30T15:44:34Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-30T20:59:21Z

✅ No public API change. #Resolved

LucGenetier · 2024-12-31T15:58:42Z

✅ No public API change. #Resolved

src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs

MikeStall · 2025-01-02T17:58:20Z

src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs

+                    {
+                        return errorRecord.Error;
+                    }
+                }


we should discuss this.

This is eagerly scanning the entire table. Can we confirm that's the language semantics?
The danger is:

that could be a lot of work upfront

that could prevent future optimizations

in paging/delegation cases, we don't download entire table . It could be a random set of rows. If there are error rows, they may or may not show up in that random set.

LucGenetier · 2025-01-02T23:38:59Z

✅ No public API change. #Resolved

MikeStall

LucGenetier · 2025-01-09T15:10:51Z

✅ No public API change. #Resolved

…d (value) multiple times.

LucGenetier · 2025-01-19T22:20:42Z

✅ No public API change. #Resolved

CarlosFigueiraMSFT · 2025-01-20T16:29:05Z

src/libraries/Microsoft.PowerFx.Core/Texl/Remove.cs

+            return base.TryGetTypeForArgSuggestionAt(argIndex, out type);
+        }
+
+        public RemoveBaseFunction(int arityMax, params DType[] paramTypes)


[minor] Can you move those function declarations after the constructor for consistency?

CarlosFigueiraMSFT · 2025-01-20T18:24:11Z

src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt

+If(true, {test:1}, "Void value (result of the expression can't be used).")
+
+>> t4
+Table()


Not sure if all of those are covered, but we should have them:

Setup:
ClearCollect(coll, {a:1}, {a:2}, {a:3}, {a:4}, {a:5}, {a:4}, {a:3}, {a:2})

Cases:
Remove(coll, [{a:2}], RemoveFlags.First) // Simple case, First (single element)
-> At the end: 1,3,4,5,4,3,2

Remove(coll, [{a:2}], RemoveFlags.All) // Simple case, All (single element)
-> At the end: 1,3,4,5,4,3

Remove(coll, [{a:2},{a:3}], RemoveFlags.First) // Simple case, First (multiple elements)
-> At the end: 1,4,5,4,3,2

Remove(coll, [{a:2},{a:3}], RemoveFlags.All) // Simple case, All (multiple elements)
-> At the end: 1,4,5,4

Remove(coll, [{a:2},{a:6}], RemoveFlags.First) // Only remove found elements, ignore others, First
-> At the end: 1,3,4,5,4,3,2

Remove(coll, [{a:2},{a:9}], RemoveFlags.All) // Only remove found elements, ignore others, All
-> At the end: 1,3,4,5,4,3

Remove(coll, ForAll(Sequence(5),{a:Value}), RemoveFlags.First) // Match all elements, First
-> At the end: 4,3,2

Remove(coll, ForAll(Sequence(5),{a:Value}), RemoveFlags.First) // Match all elements, All
-> At the end:

Remove(coll, {Value:1}, [{Value:2}])
-> Mixing record and tables should result in a compile-time error

CarlosFigueiraMSFT · 2025-01-20T18:24:48Z

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

@@ -3,6 +3,7 @@

 using System.Collections.Generic;
 using System.Data;
+using System.Linq;


CarlosFigueiraMSFT · 2025-01-20T18:25:34Z

src/strings/PowerFxResources.en-US.resx

@@ -3291,21 +3291,41 @@
    <comment>function_parameter - Second argument to the Patch function - the updates to be applied to the given rows.</comment>
  </data>
  <data name="AboutRemove" xml:space="preserve">
-    <value>Removes a specific record or records from a data source</value>
+    <value>Removes (optionally All) items from the specified 'collection'.</value>


All

all (lower case). We either use the proper enum name ('RemoveFlags.All') or treat 'all' as a common word.

CarlosFigueiraMSFT · 2025-01-20T18:26:50Z

src/strings/PowerFxResources.en-US.resx

  </data>
  <data name="ErrRemoveAllArg" xml:space="preserve">
    <value>If provided, last argument must be 'RemoveFlags.All'. Is there a typo?</value>
    <comment>{Locked=RemoveFlags.All} Error Message, RemoveFlags.All is an enum value that does not get localized.</comment>
  </data>
+  <data name="AboutRemove_collection" xml:space="preserve">
+    <value>The collection to remove rows from.</value>


The collection to remove rows from.

Suggestion: "The collection from which rows are to be removed." This avoid ending a sentence with a proposition.

CarlosFigueiraMSFT · 2025-01-20T18:27:50Z

src/strings/PowerFxResources.en-US.resx

-    <value>data_source</value>
-    <comment>function_parameter - First parameter for the Remove function. The data source that contains the records that you want to remove from. Translate this string. When translating, maintain as a single word (i.e., do not add spaces).</comment>
+  <data name="RemoveArg1" xml:space="preserve">
+    <value>collection</value>


collection

We can also use Remove to delete items from a connected data source (not only a collection)

CarlosFigueiraMSFT · 2025-01-20T19:46:48Z

src/strings/PowerFxResources.en-US.resx

+    <comment>function_parameter - Third argument for the Remove function - value indicating whether to remove all matches or only the first one.</comment>
+  </data>
+  <data name="AboutRemove_all" xml:space="preserve">
+    <value>An optional argument that specifies whether to remove all matches, or just the first one.</value>


one

Since we expect it to be a member of the RemoveFlags enumeration, we should list the possible values here (e.g.: An optional argument to specify whether to remove all matches (RemoveFlags.All) or just the first one (RemoveFlags.First).

CarlosFigueiraMSFT · 2025-01-20T19:51:59Z

#SETUP: PowerFxV1CompatibilityRules,EnableExpressionChaining,MutationFunctionsTestSetup,StronglyTypedBuiltinEnums

The "base" test Remove.txt already defines PowerFxCompatibilityRules, the cases in this file can be merged into the base one.

Refers to: src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove_V1Compact.txt:1 in 1bf13e3. [](commit_id = 1bf13e3, deletion_comment = False)

Add new connector setting _UseItemDynamicPropertiesSpecialHandling_ Remove _UseDefaultBodyNameForSinglePropertyObject_ as it was not fixing the issue as expected Special case handling when - body name is 'item' - body inner object is 'dynamicProperties' - there is only one property in inner object In that base the body will be fully flattened and we will retain the 'body' name for the parameter.

Address #2751. This unifies most of them. To scope this PR, there are subissues on 2751 for remaining issues. We have a plethora of IAsyncTexlFunction* interfaces. Want to consolidate on 1, and make it public. This Ttake the parameters on the various IAsyncTexlFunction* overloads and move them as properties on a new public `FunctionInvokerInfo` class. And then have a new IAsyncTexlFunction* that takes the invoker. And remove all the others, migrating them onto this - and fixing the various bugs/hacks along the way that would impede migration. Most fundamentally, this FunctionInvokerInfo must have access to interpreter state and so must live in the interpreter. (Whereas some of the IAsyncFunction* interfaces live in core). Successfully removes several here so we get a net reduction, and shows a path to remove the rest. This problem quickly touches other problems: - **How to map from the TexlFunction to the invoker**. This is made more complicated because we register on PowerFxConfig, but that lives in Fx.Core and can't refer to interpreter implementations. - **dll layering around Fx.Json**: Some function implementations live in Fx.Json, but that specifically _does not_ depend on interpreter (because it is included by PowerApps). - **Standard Pipeline**: how to handle default args, common error handling scenarios, etc. Today, that's still in interpreter. Whereas we really want those checks to be in the IR and shared across front-ends (and available to designer). - **Split up Library.cs and class** - we have one giant class, partial over many files, containing 100s of function impls. Just give each impl its own file, similar to how we did for TexlFunction and the static declarations. - **lack of unit testing**: Today, most of our interpreter coverage comes from .txt files. But we should have C# unit tests on interpreter classes that provide coverage on core classes, like EvalVisitor. Some related prereq fixes that will directly help here: - Can we remove runner/context from Join? and leverage LambdaValue - which already closes over these. - fix Json layering problem. - IsType/AsType _UO shouldn't live in Json. They should work on any UO. - Remove _additionalFunctions from the serviceProvider. --------- Co-authored-by: Mike <[email protected]>

LucGenetier · 2025-01-23T05:16:27Z

✅ No public API change.

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 65b5f6c to 5e675d8 Compare December 13, 2024 19:42

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 4619d74 to 8983927 Compare December 19, 2024 16:32

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 9dbde75 to 617a7fd Compare December 26, 2024 20:48

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 4ea55b9 to 42a16ef Compare December 30, 2024 20:29

anderson-joyle changed the title ~~WIP Remove function~~ Unifying Remove function with PA Dec 31, 2024

anderson-joyle marked this pull request as ready for review December 31, 2024 21:28

anderson-joyle requested a review from a team as a code owner December 31, 2024 21:28

MikeStall reviewed Jan 2, 2025

View reviewed changes

src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs Outdated Show resolved Hide resolved

MikeStall reviewed Jan 2, 2025

View reviewed changes

MikeStall previously approved these changes Jan 3, 2025

View reviewed changes

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 236b991 to 9f00032 Compare January 9, 2025 14:41

anderson-joyle and others added 6 commits January 19, 2025 15:43

initial

4e89b28

Editing test cases. WIP.

cc58019

WIP Remove function

e1ca34b

Return type based on PFxV1 flag.

af81e47

Printing better error message when PFx.V1 is active

81854f6

Fixing delegable feature.

ce31e61

anderson-joyle added 7 commits January 19, 2025 15:43

RemoveAll delegation fixes.

91db782

Covering some corner cases. Fixing a bug when removing the same recor…

d4a9d84

…d (value) multiple times.

PA errors fixing.

5fcbff8

Checkpoint. PA build was working before these changes.

fa3ac54

Analysis mode removal.

a28021b

Fix.

243b074

PR feedback

1bf13e3

anderson-joyle force-pushed the andersonf/unify/remove-function branch from 9f00032 to 1bf13e3 Compare January 19, 2025 21:43

CarlosFigueiraMSFT reviewed Jan 20, 2025

View reviewed changes

LucGenetier and others added 2 commits January 22, 2025 22:13

anderson-joyle dismissed MikeStall’s stale review via cc8c3b1 January 23, 2025 04:14

Merging

84ed0b2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unifying Remove function with PA #2776

Unifying Remove function with PA #2776

anderson-joyle commented Dec 13, 2024 •

edited

Loading

LucGenetier commented Dec 13, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 16, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 17, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 26, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 28, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 30, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 30, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 31, 2024 •

edited by anderson-joyle

Loading

MikeStall Jan 2, 2025

LucGenetier commented Jan 2, 2025 •

edited by anderson-joyle

Loading

MikeStall left a comment

LucGenetier commented Jan 9, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 19, 2025 •

edited by anderson-joyle

Loading

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT Jan 20, 2025 •

edited

Loading

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT Jan 20, 2025

CarlosFigueiraMSFT commented Jan 20, 2025

LucGenetier commented Jan 23, 2025

Unifying Remove function with PA #2776

Are you sure you want to change the base?

Unifying Remove function with PA #2776

Conversation

anderson-joyle commented Dec 13, 2024 • edited Loading

LucGenetier commented Dec 13, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 16, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 17, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 19, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 19, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 19, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 26, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 28, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 30, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 30, 2024 • edited by anderson-joyle Loading

LucGenetier commented Dec 31, 2024 • edited by anderson-joyle Loading

MikeStall Jan 2, 2025

Choose a reason for hiding this comment

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

MikeStall left a comment

Choose a reason for hiding this comment

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

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

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT Jan 20, 2025

Choose a reason for hiding this comment

CarlosFigueiraMSFT commented Jan 20, 2025

LucGenetier commented Jan 23, 2025

anderson-joyle commented Dec 13, 2024 •

edited

Loading

LucGenetier commented Dec 13, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 16, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 17, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 19, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 26, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 28, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 30, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 30, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Dec 31, 2024 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 2, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 9, 2025 •

edited by anderson-joyle

Loading

LucGenetier commented Jan 19, 2025 •

edited by anderson-joyle

Loading

CarlosFigueiraMSFT Jan 20, 2025 •

edited

Loading