From 9dbde7551d894f21ceeae9efe3f2e77d965760e5 Mon Sep 17 00:00:00 2001 From: anderson-joyle Date: Thu, 26 Dec 2024 14:45:21 -0600 Subject: [PATCH] Covering some corner cases. Fixing a bug when removing the same record (value) multiple times. --- .../Public/Values/CollectionTableValue.cs | 16 ++++-- .../Microsoft.PowerFx.Core/Texl/Remove.cs | 57 +++++-------------- .../Functions/Mutation/MutationUtils.cs | 2 +- .../ExpressionTestCases/Remove.txt | 6 +- .../MutationScripts/Remove.txt | 46 +++++++++++++-- 5 files changed, 68 insertions(+), 59 deletions(-) diff --git a/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs b/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs index 5b967c555e..b77fbbdde8 100644 --- a/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs +++ b/src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs @@ -184,7 +184,7 @@ public override DValue Last(bool mutationCopy = false) public override async Task> RemoveAsync(IEnumerable recordsToRemove, bool all, CancellationToken cancellationToken) { var ret = false; - var deleteList = new List(); + var markedToDeletion = new HashSet(); var errors = new List(); cancellationToken.ThrowIfCancellationRequested(); @@ -206,9 +206,15 @@ public override async Task> RemoveAsync(IEnumerable> RemoveAsync(IEnumerable= 3 && i == argCount - 1) + if (argCount >= 3 && i == argCount - 1 && + ((context.Features.PowerFxV1CompatibilityRules && BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) || + (!context.Features.PowerFxV1CompatibilityRules && DType.String.Accepts(argType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)))) { - if (context.AnalysisMode) - { - if (!DType.String.Accepts(argType, exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules) && - !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) - { - fValid = false; - errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrRemoveAllArg); - } - } - else - { - if (!BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[i], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) - { - fValid = false; - errors.EnsureError(DocumentErrorSeverity.Severe, args[i], TexlStrings.ErrRemoveAllArg); - } - } - - continue; - } - else - { - fValid = false; - errors.EnsureError(args[i], TexlStrings.ErrNeedRecord_Arg, args[i]); continue; } + + fValid = false; + errors.EnsureError(args[i], TexlStrings.ErrNeedRecord_Arg, args[i]); + continue; } var collectionAcceptsRecord = collectionType.Accepts(argType.ToTable(), exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules); @@ -351,7 +333,7 @@ public override bool ArgMatchesDatasourceType(int argNum) } public RemoveAllFunction() - : base("Remove", TexlStrings.AboutRemove, FunctionCategories.Behavior, DType.EmptyTable, 0, 2, 3, DType.EmptyTable, DType.EmptyTable, DType.String) + : base("Remove", TexlStrings.AboutRemove, FunctionCategories.Behavior, DType.EmptyTable, 0, 2, 3, DType.EmptyTable, DType.EmptyTable) { } @@ -425,25 +407,12 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp } } - if (args.Length == 3) + if (args.Length == 3 && + ((context.Features.PowerFxV1CompatibilityRules && !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) || + (!context.Features.PowerFxV1CompatibilityRules && !DType.String.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)))) { - if (context.AnalysisMode) - { - if (!DType.String.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules) && - !BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) - { - fValid = false; - errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg); - } - } - else - { - if (!BuiltInEnums.RemoveFlagsEnum.FormulaType._type.Accepts(argTypes[2], exact: true, useLegacyDateTimeAccepts: false, usePowerFxV1CompatibilityRules: context.Features.PowerFxV1CompatibilityRules)) - { - fValid = false; - errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg); - } - } + fValid = false; + errors.EnsureError(DocumentErrorSeverity.Severe, args[2], TexlStrings.ErrRemoveAllArg); } returnType = context.Features.PowerFxV1CompatibilityRules ? DType.Void : collectionType; diff --git a/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs b/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs index f57c52bc59..8e33fe5476 100644 --- a/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs +++ b/src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs @@ -78,7 +78,7 @@ public static async Task RemoveCore(FormulaType irContext, Formula if (arg is TableValue tableValue) { - var errorRecord = tableValue.Rows.First(row => row.IsError); + var errorRecord = tableValue.Rows.FirstOrDefault(row => row.IsError); if (errorRecord != null) { return errorRecord.Error; diff --git a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt index ecb4352eb1..df1ce9d7f4 100644 --- a/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt +++ b/src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt @@ -4,13 +4,13 @@ // Wrong arguments >> Remove(t1, r1,"Al"); -Errors: Error 14-18: If provided, last argument must be 'RemoveFlags.All'. Is there a typo?|Error 0-6: The function 'Remove' has some invalid arguments. +Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 14-18: Cannot use a non-record value in this context: '"Al"'. >> Remove(t1, r1,""); -Errors: Error 14-16: If provided, last argument must be 'RemoveFlags.All'. Is there a typo?|Error 0-6: The function 'Remove' has some invalid arguments. +Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 14-16: Cannot use a non-record value in this context: '""'. >> Remove(t1, r1, r1, r1, r1, r1, r1, "Al"); -Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 35-39: If provided, last argument must be 'RemoveFlags.All'. Is there a typo? +Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 35-39: Cannot use a non-record value in this context: '"Al"'. >> Remove(t1, "All"); Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 11-16: Cannot use a non-record value in this context: '"All"'. diff --git a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt index ead47ca656..52478926c4 100644 --- a/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt +++ b/src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt @@ -48,24 +48,58 @@ Errors: Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Err >> 4;t1 Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) +>> Set(t3, t1) +Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +// Removing multiple rows with the same values. +>> Remove(t3, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}) +If(true, {test:1}, "Void value (result of the expression can't be used).") + +>> 0;t3 +Table({a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +>> 0;Set(t3, t1) +Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +>> Remove(t3, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)}, RemoveFlags.All) +Error(Table({Kind:ErrorKind.NotFound},{Kind:ErrorKind.NotFound})) + +>> 1;t3 +Table({a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +>> 1;Set(t3, t1) +Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +>> Remove(t3, t3) +If(true, {test:1}, "Void value (result of the expression can't be used).") + +>> 2;t3 +Table() + +>> 2;Set(t3, t1) +Table({a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:true,b:"hi",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hello",c:DateTime(2024,1,1,0,0,0,0)},{a:false,b:"hi",c:DateTime(2024,1,1,0,0,0,0)}) + +>> Remove(t3, t3, RemoveFlags.All) +Error(Table({Kind:ErrorKind.NotFound},{Kind:ErrorKind.NotFound})) + // Remove propagates error. >> Remove(t1, If(1/0<2, {a:true,b:"hello",c:DateTime(2024,1,1,0,0,0,0)})) Error({Kind:ErrorKind.Div0}) ->> Set(t3, Table({a:{aa:{aaa:true,bbb:true}}})) +>> Set(t4, Table({a:{aa:{aaa:true,bbb:true}}})) Table({a:{aa:{aaa:true,bbb:true}}}) ->> Remove(t3, {a:{aa:{aaa:true}}}) +>> Remove(t4, {a:{aa:{aaa:true}}}) Errors: Error 0-6: The function 'Remove' has some invalid arguments.|Error 11-30: Missing column. Your formula is missing a column 'a.aa.bbb' with a type of 'Boolean'.|Error 11-30: Invalid argument type. Expecting a Record value, but of a different schema.|Error 11-30: Missing column. Your formula is missing a column 'aa.bbb' with a type of 'Boolean'. ->> Remove(t3, {a:{aa:{aaa:true,bbb:false}}}) +>> Remove(t4, {a:{aa:{aaa:true,bbb:false}}}) Error({Kind:ErrorKind.NotFound}) ->> Remove(t3, {a:{aa:{aaa:true,bbb:false}}}, RemoveFlags.All) +>> Remove(t4, {a:{aa:{aaa:true,bbb:false}}}, RemoveFlags.All) Error({Kind:ErrorKind.NotFound}) ->> Remove(t3, {a:{aa:{aaa:true,bbb:true}}}) +>> Remove(t4, {a:{aa:{aaa:true,bbb:true}}}) If(true, {test:1}, "Void value (result of the expression can't be used).") ->> t3 +>> t4 Table() \ No newline at end of file