-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Expressions] Use table column ID instead of name when set #99724
Changes from 9 commits
03563b4
41789fb
628cf8d
5413d7a
468cfcf
d222796
9f4d647
7f1df87
969ffd9
8d04687
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -130,10 +130,11 @@ export const math: ExpressionFunctionDefinition< | |
throw errors.emptyExpression(); | ||
} | ||
|
||
// Use unique ID if available, otherwise fall back to names | ||
const mathContext = isDatatable(input) | ||
? pivotObjectArray( | ||
input.rows, | ||
input.columns.map((col) => col.name) | ||
input.columns.map((col) => col.id) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is meant by fallback to names ? the fact that name will be used for columnId when id was not provided (to map_column for example ?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, if no |
||
) | ||
: { value: input }; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ids should be unique, if we find existing column with requested new column id we should throw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo the logic should be:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppisljar Column IDs are guaranteed to be unique with this logic, I am not seeing any case where you could get a duplicate ID. Also, this logic lets you rewrite a column by ID, this is shown in some of the test cases. For example
mapColumn id="existing" name="New name" expression={math "existing + 2"}
is a valid expression that updates the data in place.