Skip to content

Commit

Permalink
Covering some corner cases. Fixing a bug when removing the same recor…
Browse files Browse the repository at this point in the history 
…d (value) multiple times.
  • Loading branch information
@anderson-joyle
anderson-joyle committed Dec 26, 2024
1 parent ee8e4b8 commit 9dbde75
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 59 deletions.
16 changes: 11 additions & 5 deletions src/libraries/Microsoft.PowerFx.Core/Public/Values/CollectionTableValue.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ public override DValue<RecordValue> Last(bool mutationCopy = false)
public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<FormulaValue> recordsToRemove, bool all, CancellationToken cancellationToken)
{
var ret = false;
var deleteList = new List<T>();
var markedToDeletion = new HashSet<T>();
var errors = new List<ExpressionError>();

cancellationToken.ThrowIfCancellationRequested();
Expand All @@ -206,9 +206,15 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula

if (await MatchesAsync(dRecord.Value, recordToRemove, cancellationToken).ConfigureAwait(false))
{
found = true;

deleteList.Add(item);
if (markedToDeletion.Contains(item))
{
continue;
}
else
{
found = true;
markedToDeletion.Add(item);
}

if (!all)
{
Expand All @@ -224,7 +230,7 @@ public override async Task<DValue<BooleanValue>> RemoveAsync(IEnumerable<Formula
}
}

foreach (var delete in deleteList)
foreach (var delete in markedToDeletion)
{
_sourceList.Remove(delete);
ret = true;
Expand Down
57 changes: 13 additions & 44 deletions src/libraries/Microsoft.PowerFx.Core/Texl/Remove.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,34 +119,16 @@ public override bool CheckTypes(CheckTypesContext context, TexlNode[] args, DTyp

if (!argType.IsRecord)
{
if (argCount >= 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);
Expand Down Expand Up @@ -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)
{
}

Expand Down Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/libraries/Microsoft.PowerFx.Interpreter/Functions/Mutation/MutationUtils.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public static async Task<FormulaValue> 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;
Expand Down
6 changes: 3 additions & 3 deletions src/tests/Microsoft.PowerFx.Core.Tests.Shared/ExpressionTestCases/Remove.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"'.
Expand Down
46 changes: 40 additions & 6 deletions src/tests/Microsoft.PowerFx.Interpreter.Tests.Shared/MutationScripts/Remove.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()

0 comments on commit 9dbde75

Please sign in to comment.