microsoft / microsoft/Power-Fx

SortByColumns is confused by missing fields from UDF

Open
#2,973 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
C#
Stars
3.4k
Forks
358
Avg merge
10h 34m
Merged PRs (30d)
3

Description

Note the runtime error when the Age column is omitted.

>> Person := Type( { Name: Text, Age: Number } );

>> People := Type( [ Person ] );

>> SortedPeople(list:People):People = SortByColumns(list, "Name", SortOrder.Ascending, "Age", SortOrder.Ascending);

>> SortedPeople([{Name:"John", Age:30},{Name:"Jane", Age:40}])

  Age   Name
===== ======
  40    Jane
  30    John

>> SortedPeople([{Name:"John"},{Name:"Jane"}]) // BUG!
<Error: The specified column 'Age' does not exist or is an invalid sort column type.>

>> SortedPeople([{Name:1},{Name:2}])

  Age   Name
===== ======
        1
        2

>> IR(SortedPeople([{Name:"John"},{Name:"Jane"}]))
SortedPeople:*[Age:n, Name:s](Table:*[Name:s]({Name: "John":s}, {Name: "Jane":s}))

>> IR(SortedPeople([{Name:1},{Name:2}])) // numbers coerce to strings
SortedPeople:*[Age:n, Name:s](AggregateCoercion(TableToTable, Name <- DecimalToText:s(ScopeAccess(Scope 1, Name))))

If you look in the interpreter for the bug case, the type of Arg0 to SortByColumns doesn't include Age. The argument is passed through unmodified through the UDF parameter. By using numbers in the second example, a coercion is required, which brings in the Age column.

What is the expectation here? Should an AggregateCoercion have been inserted in the bug case since the type of the tables wasn't exactly the same? In general, are missing columns acceptable throughout the system?

Or should SortByColumns have assumed that some columns referenced may not be there and treat them as Blank? This can't be done directly as it is impossible to differentiate a missing column name from an invalid column name, but can be done roundabout by using the column names of the result from SortByColumns in the IRContext instead of the first argument.

It is tempting just to patch SortByColumns, but that is just the case I found. Is this a more general problem?

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in the interpreter around SortByColumns and inspect how Arg0 types, UDF parameters, AggregateCoercion, and IRContext handle missing columns. Reproduce the examples in the issue, especially the omitted Age field, then determine whether coercion or Blank handling is the intended general behavior. Done means the behavior is decided and the regression is covered for the reported cases.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
compilers
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.