SCAL-257337: Added sortOptions to getUnderlyingDataForPoint - #678
tushardeepakts wants to merge 6 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces sorting capabilities to the getUnderlyingDataForPoint method in AnswerService by adding a new SortOptions interface, an updateSort GraphQL mutation, and the applySort helper method. Corresponding unit tests have also been added to verify the sorting behavior. The review feedback suggests adding defensive checks within applySort to handle missing or invalid inputs (such as undefined source columns or unrecognized column names) to prevent runtime TypeErrors and improve robustness.
commit: |
tushardeepakts
changed the title
| /** | ||
| * Describes how to sort a column when fetching data. Pass an array of these to | ||
| * sort by multiple columns, in priority order. | ||
| * @version SDK: 1.52.0 | ThoughtSpot Cloud: 26.9.0.cl |
There was a problem hiding this comment.
The SDK version I have taken from package.json, and TS cloud version I have calculated from the formula mentioned in Claude.md file :
For SDK 1.N.x the Cloud version is 26.(N-43).0.cl
| private async applySort(sortOptions: SortOptions[], sourceDetail: any) { | ||
| const sortDetails = sortOptions.map((sort) => ({ | ||
| columnId: getGuidsFromColumnNames(sourceDetail, [sort.columnName]).values().next().value, | ||
| sortType: sort.ascending ? 'ASCENDING' : 'DESCENDING', |
There was a problem hiding this comment.
make a enum for this
There was a problem hiding this comment.
These are internal wire-contract values used once in a ternary. An enum here would be unnecessary. The public API only exposes ascending: boolean; ASCENDING/DESCENDING are just internal GraphQL values.
| } | ||
|
|
||
| if (sortOptions?.length) { | ||
| await unaggAnswerSession.applySort(sortOptions, sourceDetail); |
There was a problem hiding this comment.
sort should supported by api right?
There was a problem hiding this comment.
The API can sort. applySort is not sorting — it's just a translator so a customer can trigger that API without knowing its details.
Customer knows: column name + ascending: true/false
API wants: answer column id + "ASCENDING"/"DESCENDING" on the right session
applySort converts the first into the second, then calls the API. That's it. Same reason addFilter and addColumns exist — each is a friendly wrapper over one backend call so customers don't touch raw GraphQL.
Changed its name to "adaptSorting" which makes more sense
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces sorting capabilities when fetching underlying data in AnswerService by adding a SortOptions interface, updating the getUnderlyingDataForPoint method, and implementing the updateSort GraphQL mutation. Feedback on these changes highlights critical robustness issues in adaptSorting and getGuidToColumnIdMap that could lead to runtime TypeError exceptions due to missing defensive checks. Additionally, a JSDoc tag ordering violation was identified in getUnderlyingDataForPoint where the @version tag is incorrectly placed after the @example block.
| private async adaptSorting(sortOptions: SortOptions[], sourceDetail: any) { | ||
| const { answer } = await this.executeQuery(queries.getAnswer, {}); | ||
| const guidToColumnId = getGuidToColumnIdMap(answer); | ||
| const sortOrder = sortOptions.map((sort) => { | ||
| const guid = getGuidsFromColumnNames(sourceDetail, [sort.columnName]) | ||
| .values().next().value; | ||
| return { | ||
| columnId: guidToColumnId.get(guid), | ||
| sortType: sort.ascending ? 'ASCENDING' : 'DESCENDING', | ||
| }; | ||
| }); | ||
| return this.executeQuery( | ||
| queries.updateSort, | ||
| { | ||
| sortOrder, | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The current implementation of adaptSorting calls getGuidsFromColumnNames which is prone to throwing a TypeError if any of the provided column names in sortOptions do not exist in the data source (since it attempts to access .id on an undefined column object).
Additionally, destructuring const { answer } = await this.executeQuery(...) can throw a TypeError if the query execution fails or returns an empty/undefined response.
We can make this method significantly more robust and defensive by:
- Safely checking the query response before destructuring.
- Finding the column directly from
sourceDetail.columnsusing safe optional chaining, which avoids calling the crash-pronegetGuidsFromColumnNameshelper. - Filtering out any invalid or unmapped columns instead of letting them cause runtime exceptions.
private async adaptSorting(sortOptions: SortOptions[], sourceDetail: any) {
const response = await this.executeQuery(queries.getAnswer, {});
const answer = response?.answer;
if (!answer) {
return null;
}
const guidToColumnId = getGuidToColumnIdMap(answer);
const sortOrder = sortOptions
.map((sort) => {
const column = sourceDetail?.columns?.find(
(col: any) => col.name.toLowerCase() === sort.columnName.toLowerCase(),
);
const guid = column?.id;
const columnId = guid ? guidToColumnId.get(guid) : undefined;
if (!columnId) {
return null;
}
return {
columnId,
sortType: sort.ascending ? 'ASCENDING' : 'DESCENDING',
};
})
.filter((item): item is { columnId: string; sortType: string } => !!item);
if (sortOrder.length === 0) {
return null;
}
return this.executeQuery(
queries.updateSort,
{
sortOrder,
},
);
}| function getGuidToColumnIdMap(answer: any): Map<string, string> { | ||
| const tableViz = answer.visualizations.find((viz: any) => viz.columns); | ||
| return new Map( | ||
| tableViz.columns.map( | ||
| (col: any) => [col.column.referencedColumns[0].guid, col.column.id], | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
The newly added helper function getGuidToColumnIdMap is not defensively programmed and can easily throw runtime TypeError exceptions under several conditions:
- If
answeroranswer.visualizationsis undefined. - If no visualization with
columnsis found (makingtableVizundefined). - If any column does not have a
referencedColumnsarray or if the array is empty (causingreferencedColumns[0]to be undefined).
We should use optional chaining and safe iteration to ensure this helper is completely crash-proof.
function getGuidToColumnIdMap(answer: any): Map<string, string> {
const tableViz = answer?.visualizations?.find((viz: any) => viz.columns);
const columns = tableViz?.columns || [];
const map = new Map<string, string>();
for (const col of columns) {
const guid = col?.column?.referencedColumns?.[0]?.guid;
const id = col?.column?.id;
if (guid && id) {
map.set(guid, id);
}
}
return map;
}| * automatically passed. | ||
| * @param outputColumnNames | ||
| * @param selectedPoints | ||
| * @param sortOptions Optional columns to sort the underlying data by. |
There was a problem hiding this comment.
The JSDoc block for getUnderlyingDataForPoint violates the canonical tag ordering rule specified in the repository style guide. Currently, the @version tag (on line 359) is placed after the @example block. According to the style guide, @version must be placed before @param and @example.
Please reorder the tags so that @version is placed before @param.
This branch has not been deployed
2 participants
Summary
Adds an optional
sortOptionsparameter so customers can sort the raw underlying data behind a clicked chart point.Changes
SortOptionstype ({ columnName; ascending }), exported fromindex.ts.sortOptionsparameter togetUnderlyingDataForPoint(cols, points?, sortOptions?).updateSortmutation.applySort()helper.Video:
Screen.Recording.2026-09-21.at.2.53.20.PM.mov