fix(select): select component sort functionality on certain options

[corbinrobb]

SUMMARY

Working on fixing the issues with row limits, series limits, and other select options being sorted by their label instead of their number value. These are in the Explore view but the select component is used all over the place.

TL;DR
Some options are being sorted funky and there is other weirdness happening. I have implemented a fix that will sort the numbers but there may be better ways to do it. I am currently enumerating over files where select is being used and I am working on a solution that will satisfy all of options that get passed while not unnecessarily bloating or increasing the complexity of the component or codebase.

What's happening:

So far I have figured out that the defaultSortOperator from Select (superset-frontend/src/components/Select/Select.tsx) (pictured below) is sorting the options being passed in to by their label if the options have a label that is typeof 'string'. The problem is that the numbers that are being passed in with a string label and a number value ex: { label: '20', value: 20 }.
When the Select is inside of a SelectControl (superset-frontend/src/explore/components/controls/SelectControl.jsx) like it is when it is being used for viz plugins, the numbers are always being passed in with a label thanks to formatting.

So the below function is comparing a.label to b.label and sorting the numbers by their labels because from what I have been able to find, there aren't any (many?) scenarios that numbers are being passed in without a label.

const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
  if (typeof a.label === 'string' && typeof b.label === 'string') {
    return a.label.localeCompare(b.label);
  }
  if (typeof a.value === 'string' && typeof b.value === 'string') {
    return a.value.localeCompare(b.value);
  }
  return (a.value as number) - (b.value as number);
};

Discussion

If you are like me you are probably thinking, "Okay cool, then let's change that default operator to compare by value when the value is a number". The problem I found with this is that there is at least one other implementation of Select ( I currently only know of the Dataset selection in the Charts view) where label is a string with a title and the value is a number. So doing that changes those options to be sorted by their number value causing the options to appear out of order. I have not looked into what is going on there but I am assuming that the numbers serve a good purpose like an index for an API call to the backend.

Current Fix (New Implementation in Update 1):

The fix that has makes the most sense to me, with my current knowledge and understanding of what's happening, is to utilize the propertyComparator function right below defaultSortComparator as the default sort function used and add an optional property called sortByProperty to Select that's default value is 'label' that I call and assign to sortOperator property on Select like so sortComparator = propertyComparator(sortByProperty). This will have the default sorting be done by label and allow the option to pass in a string property value to sort by a specific property.

export const propertyComparator =
  (property: string) => (a: AntdLabeledValue, b: AntdLabeledValue) => {
    if (typeof a[property] === 'string' && typeof b[property] === 'string') {
      return a[property].localeCompare(b[property]);
    }
    return (a[property] as number) - (b[property] as number);
  };

This works for the cases where we getting viz plugins like MixedTimeSeries as long as we add the sortBy value into the control configurations for the data visualizations as well as pass the prop down into Select after it is passed into SelectControl.

This also would allow us to remove the defaultSortComparator function and also remove where propertyComparator is being imported into some components and just pass in the property to Select as a string. I have had defaultSortComparator commented out for a while and haven't noticed anything but certainly doesn't mean something didn't break. I am planning to find every implementation of Select and check all the options being passed in. I will also manually test this.

No matter what ends up being the solution to this, I believe it is a must to add more tests to all of the current tests it has to verify that it works with numbers as expected.

IMPORTANT (Addressed in Update 1)

I am not sure about my solution and the more that I am looking into this, the more I am noticing other issues and getting more ideas. All of this directly involves the Select component and its sort functionality, so it feels appropriate to attempt to fix it or at the very least try to get an idea of what's happening so I can help somebody else do so. Also, I have not gone through and added this property to all the viz configurations that would use it yet because I am still researching.

• The number arrays that are being passed in are constant variables that are starting out sorted and then are being unsorted by the Select component once they get passed in. It might make sense to have the sorting be optional because not all options being rendered make sense when sorted. Like for instance Dates, and that brings me to the next thing.
• Dates are sent in sorted from smallest time to largest time and are then sorted by label and displayed unsorted. I am of the opinion that dates should go from smallest to largest but with these, we can't sort by either label or value because neither would work or make sense. This is what the label and value look like for some {label: "Hour", value: "PT1H"}, {label: "Minute", value: "PT1M"}. I feel these should not be sorted once they get into the Select or we could create a constant object that holds key:value pairs of the date option value and a corresponding size and then use that within the comparator function. But I don't like that idea because it would still involve having to know if the options are dates once they go in or we would have to write logic to check the options if they are dates and that feels, I dunno, gross and stupid? (I am very mature)
• There is another bug with the Select component where the sort order breaks upon selection if it has multiple selections enabled. On single mode it will close the component and the next time you open it it will be in order, but with multiple on it stays open after selecting an option and immediately changes the options to be out of order. This is weird and unexpected and it feels like it might have something to do with the sorting. I will attempt to fix this as well.

BEFORE

AFTER

TESTING INSTRUCTIONS

Navigate to Charts and open up any chart that has selections with number, date, or groupby dropdown selections like Time Series, or Table.

Open and see the order of the options

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[corbinrobb]

@hughhhh @michael-s-molina I was told that I should tag both of you. If you have any thoughts or suggestions I am very open to hearing them. I know that Michael wrote the logic and functions for the sorting in the PR to fix the order of select when selecting. So I imagine you would know if I am missing information or I am breaking something that I shouldn't

[corbinrobb]

Update 1

Okay big update here. As I was going through and adding all of the sortByProperty tags into the viz plugin configurations, I noticed that it was by far much more common for the options that are being passed into the Select component to be in it's intended order as it gets passed in.

After getting a much better feel for the component and what is going on in it, I came to the conclusion that passing in a new property for the configuration to define what property the options are getting sorted by was a backwards solution. The problem isn't the sorting, it was the assumption that the options were coming in sorted by one of their properties. Having the options come in and then sorting them by a property was assuming that this wasn't putting them out of order.

Fix

The fix I have come up with that appears to work across the board for the numbers, dates, and every set of options that are passed into the Select, is to not sort by a property by default. It might seem a little weird but bear with me because I feel like it makes a lot of sense.

From what I could see main purpose of most of the sorting in the component was using it as a way to have the options go back to their original order once an option is deselected instead of having it sit at the top of the options. That makes sense but since the options were in a order that isn't easily replicated by comparing any of its values, I feel it made the most sense to create a dictionary or hash map of that original order and sort the values by that. Below are my functions were I implement this.

const getInitialIndexes = (options: OptionsType) =>
  options.reduce((a, c, i) => ({ ...a, [c.value]: i }), {});

const sortByInitialIndexes = (
  options: OptionsType,
  originals: { value?: number },
) => options.sort((a, b) => originals[a.value] - originals[b.value]);

What is going on here is the options are getting passed to getInitialIndexes and it is returning a hashmap that has a key:value pair of the options value and the index of the option in the array. This gives us the initial indexes of the options and allows us to grab them in O(1) time using a property that all the options have. With this, we can use those index values as the thing that we compare in our sort functions.

This happens in sortByInitialIndexes and is done by taking a param of the options array and an original indexes hash and then using the indexes as the comparison. I like the way that this works because it is always comparing numbers and it doesn't matter whether the values are strings or numbers. It is only concerned about the original indexes and does not need to try to decide what the best way sort the type of data.

I figured that since this hash needs to be created once and only gets read after that, it made sense to store it in a reference. We can then pass this ref into the sort function and we have the options back in their original order.

Here is an example where sort was being used in the handleTopOptions function.

const sortedOptions = [
      ...sortByInitialIndexes(topOptions, optionsInitialOrder.current),
      ...sortByInitialIndexes(otherOptions, optionsInitialOrder.current),
    ];
    if (!isEqual(sortedOptions, selectOptions)) {
      setSelectOptions(sortedOptions);
    }

Here is where the selected options are being put at the top and the unselected options are being put below them. They were both being sorted by defaultSortComparator before which was sorting them by label. Now with this we are putting the selected options at the top and then the rest of the options are just being sorted in the value that they were already in.

Here is where the same thing is happening when options are deselected

if (options.length > 0) {
        const sortedOriginal = sortByInitialIndexes(
          selectOptions,
          optionsInitialOrder.current,
        );
        setSelectOptions([...options, ...sortedOriginal]);
      }

The same idea as the other but now the deselected option/options are being sorted to the order it was in before being selected.

This seems to work really well but there was a set of options that is in a lot of viz plugins that use the selection 'group by' and that is passed in unsorted and it made sense to add an option to have those be sorted by a certain value. So I added a boolean prop that lets the component know that the initial options should be sorted by this property. It then sorts the options by that property and sets the options to it while adding that initial sorted order into the hashmap.

const [selectOptions, setSelectOptions] = useState<OptionsType>(
    sortOptions ? initialOptions.sort(sortComparator) : initialOptions,
  );

const optionsInitialOrder = useRef(getInitialIndexes(initialOptions));

This sets the initial options to be sorted and sets the selectOptions state to start at that initial value.

Conclusion

So that's pretty much it, I still would like to add some tests and I also need to do some small changes that I noticed as I was writing this. As always, there might be something I am not seeing but this fix seems to work really well so far. Oh and I still left the defaultSortOperator commented out in there. I wanted to leave it as a reference for anybody who wants to help me decide if I should remove it or keep it around and still use it.

Before

After

[michael-s-molina]

@corbinrobb Thank you for writing the PR with such a great level of detail. I do have some observations that can help with the context of the component and the requirements that we're trying to achieve:

The fix that has makes the most sense to me, with my current knowledge and understanding of what's happening, is to utilize the propertyComparator function right below defaultSortComparator as the default sort function used and add an optional property called sortByProperty to Select that's default value is 'label' that I call and assign to sortOperator property on Select like so sortComparator = propertyComparator(sortByProperty). This will have the default sorting be done by label and allow the option to pass in a string property value to sort by a specific property.

The important point here is to preserve the sortComparator function as a parameter to the component. This flexibility is important because we can have unique sorting requirements. One example would be the dates where the value contains a specific string that should be interpreted to provide the correct sorting order. Another example would be using multiple properties to calculate the sorting. The defaultSortComparator is meant to be simple and handle the most common cases. The propertyComparator is a helper function that provides one common pattern of sorting that compares a single property using the equal sign. We can have other flavors of helpers for dates, multiple properties, etc. We should probably move the propertyComparator outside of the Select component.

From what I could see main purpose of most of the sorting in the component was using it as a way to have the options go back to their original order once an option is deselected instead of having it sit at the top of the options. That makes sense but since the options were in a order that isn't easily replicated by comparing any of its values, I feel it made the most sense to create a dictionary or hash map of that original order and sort the values by that.

This solution was discussed during development but we have a requirement that invalidated this approach. The Select component can be asynchronous, paginated, and with search capabilities. That means that the order that the results are being fetched is not necessarily the correct order of the data. Imagine that we have a Select with the names of people. When we open the Select the first page loads the people with names that start with the letter A. If we scroll down, the next page is loaded, let's say with names that start with the letter B. So far the order is the same, but now the user searches for W and later for L. The order that we get the results is different from the order of the data. As we also don't know what's the sorting algorithm, we can't sort this dictionary. That's why we need the sortComparator, each place when the Select is applied knows what's the correct sorting algorithm and can configure it appropriately.

Dates are sent in sorted from smallest time to largest time and are then sorted by label and displayed unsorted. I am of the opinion that dates should go from smallest to largest but with these, we can't sort by either label or value because neither would work or make sense. This is what the label and value look like for some {label: "Hour", value: "PT1H"}, {label: "Minute", value: "PT1M"}.

The problem is that the numbers that are being passed in with a string label and a number value ex: { label: '20', value: 20 }. When the Select is inside of a SelectControl (superset-frontend/src/explore/components/controls/SelectControl.jsx) like it is when it is being used for viz plugins, the numbers are always being passed in with a label thanks to formatting.

I think for these examples, a possible solution would be to create another set of comparator helpers that deal with these specific scenarios and pass them when needed. Similar to propertyComparator.

No matter what ends up being the solution to this, I believe it is a must to add more tests to all of the current tests it has to verify that it works with numbers as expected.

You're totally right here! Tests are essential since this component is widely used throughout the application and every change is of high risk.

I'm currently on PTO but I'll try to keep an eye on this PR and help any way I can. Thanks again for bringing up these points of improvement and writing such an amazing description.

[codecov]

Codecov Report

Merging #17638 (499ae7f) into master (b5d13d7) will increase coverage by 0.21%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master   #17638      +/-   ##
==========================================
+ Coverage   68.55%   68.77%   +0.21%     
==========================================
  Files        1602     1597       -5     
  Lines       65354    65227     -127     
  Branches     6994     6956      -38     
==========================================
+ Hits        44806    44857      +51     
+ Misses      18666    18483     -183     
- Partials     1882     1887       +5     

Flag	Coverage Δ
javascript	57.45% <60.00%> (+0.34%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx	38.53% <0.00%> (-0.36%)	⬇️
...ns/legacy-plugin-chart-heatmap/src/controlPanel.ts	80.00% <ø> (ø)
...nd/plugins/plugin-chart-table/src/controlPanel.tsx	21.34% <0.00%> (-0.50%)	⬇️
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx	67.15% <66.66%> (-0.26%)	⬇️
superset-frontend/src/components/Select/Select.tsx	86.33% <100.00%> (ø)
.../src/explore/components/controls/SelectControl.jsx	60.34% <100.00%> (+1.41%)	⬆️
...rset-ui-core/src/connection/SupersetClientClass.ts	90.76% <0.00%> (-7.54%)	⬇️
...rc/explore/components/ExploreChartHeader/index.jsx	46.57% <0.00%> (-0.65%)	⬇️
...packages/superset-ui-core/src/query/types/Query.ts	100.00% <0.00%> (ø)
...ugins/legacy-plugin-chart-calendar/src/Calendar.js	0.00% <0.00%> (ø)
... and 20 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b5d13d7...499ae7f. Read the comment docs.

[corbinrobb]

Update 2

Thank you everyone for all of the comments and help while looking at this with me! Also thank you Michael for the very detailed explanation, it really helped me out.

Alrighty, I reverted everything that I did within the Select component because there was a simpler way that I did not notice. Sorry about this ridiculous PR. I thought the solution would be simple but the component is widely used and versatile so it took me a whole lot of searching, reading, and manual testing.

Problem

Mostly the same. There are Select components being made that are being passed options from SelectControl and they are being constructed from arrays of mostly static and mostly hardcoded data. From all of the ones that I have seen the majority seem to expect that data to retain its order.

I have come to this conclusion after looking at the nearly 200 SelectControl configuration objects/functions that are being used to make a lot Select components in the explore dashboard. I began writing a list of them to share where they are and where the data is created or exists, but it was incredibly tedious and I gave up after about 80.

I will just tell you how to find them. All you need to do is do a project-wide search for SearchControl, and then exclude the translation files or just scroll past them. You should get a little more than 200 hits where you will find objects that look something like this

{
            name: 'subdomain_granularity',
            config: {
              type: 'SelectControl',
              label: t('Subdomain'),
              default: 'day',
              choices: formatSelectOptions([
                'min',
                'hour',
                'day',
                'week',
                'month',
              ]),
              description: t(
                'The time unit for each block. Should be a smaller unit than ' +
                  'domain_granularity. Should be larger or equal to Time Grain',
              ),
            },
          },

The data that will make up the Select options are in choices, options, or in a mapStateToProps. A lot of them get formatted with a function like the one above called formatSelectOptions This creates an array that is ready to be passed to Select so it can make some options. Normally makes them look like this [value, String(value)].

Most of the data for these are hard-coded values like in the example above. While the data is not all unique, there still are separate arrays being made for most and this is especially true for legacy plugins. Some of the data is unsorted datasource columns and those are the ones I felt needed sortComparators added to them. Everything else appears in order or at least in the intended order.

If you want to confirm how all of this looks you can do the search for SelectControls and you will be able to see for yourself.

There are Select components not being made within SelectControl like popovers, modals, and other things. From what I could see all of those had been accounted for and are displayed in an appropriate order.

New FIx

Okay. When the fix for the top options going back to their correct positions after deselection was added, I saw that some objects were added an order key with its array index as the value. If I am not mistaken, this was done to preserve the order for these data arrays because there are values like operators, dates, and other such values.

example of data with order keys

These order indexes function the same way as the initial options index hashmap that I was using in a previous attempt and I'm all for using them for the data that gets passed into SelectControls when the plugins are made.

In SelectControl we have a method that iterates the formatted data array and returns objects for the Select component to build options.

This is the place where the data I am concerned with pass through. c is an item in a choices array

     if (Array.isArray(c)) {
          const [value, label] = c.length > 1 ? c : [c[0], c[0]];
          return {
            value,
            label,
          };
        }

Add i for index on the map function, check if there is not a sortComparator(more on that in a moment) and if there is not then add an order key to sort in order.

Since most of the chart/graph plugins are sending data that is either sorted or being passed in its intended order, this fixes the out-of-order data for the select dropdowns that are being made with SelectControl.

Now we can just add sortComparator where they make sense, like the columns from a datasource. And those will be sorted as we would like.

I chose not to import the propertyComparator for these and just used anon arrow functions. I did this because of the distance from the component and because the other imports in those files were from short distances.

I do not have solid reasoning for doing it this way, it was just a gut feeling that I should. If anybody disagrees with that I am happy to change it.

The last thing is setting a conditional in SelectControl that checks for a sortComparator value and if one doesn't exist then sort by order using propertyComparator

sortComparator: this.props.sortComparator || propertyComparator('order')

That's the fix! There is not a whole lot going on and there are other ways but I feel like this is quick and simple.

I did get the implementation that I was making earlier working and got it up and going it working with the pagination and everything else but I abandoned it last minute for this fix because it is simpler and requires fewer changes.

Other little fixes done on my journey

legacy heatmap chart

There was a bug with the legacy heatmap chart within two of the Select components named xscale_interval and yscale_interval. This was from the config passing a default value of "1" and the choices data object passed uses numbers and start at 1.

The Select component doesn't know where to put the one and will duplicate it after clicking other options. Setting default to the number one instead of the string fixes this.

Before

After

AdhocFilterEditPopoverSimpleTabContent

The operators Select was not passed the order key or the sortComparator and the options appeared out of order. Then the Select holding the column values were being sorted by the label which worked when the values were string but sorted them out of order on numbers.

I added an order key and comparator for the operators Select. Then for the column values Select I added a condition that will sort by the values when they are numbers and by label if not.

Before

After

Select sorting initial state

Caught this while writing this and I meant to have it in the last commit. I will push right after this comment.

The initial options being passed directly into the state works fine for most cases but when you can select multiple options it will do a jump while the dropdown is open and it throws all the options back into what the state's initial options were. I am not sure what is causing the jump and I couldn't get it to stop.

Anyways, having it jump from ordered to unordered is confusing so I just have the options sorted when they are initialized in the selectOptions useState hook.

const [selectOptions, setSelectOptions] = useState<OptionsType>(
    initialOptions.sort(sortComparator),
  );

Still jumps from the selected options being at the top to having them back in order.

It isn't great but it is better than having it jump out of order.

Before

After

[michael-s-molina]

@corbinrobb Thanks for the update and for fixing the initial sorting. I won't be able to test it but I agree with the changes.

@geido can you review it? If you think it's ok to merge it then I'm ok too.

[AAfghahi]

/testenv up

[github-actions]

@AAfghahi Ephemeral environment spinning up at http://35.161.208.185:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[AAfghahi]

This looks really great, happy to approve once you add some testing as we discussed.

[eschutho]

Looks great @corbinrobb!

[geido]

LGTM! Thanks for the awesome effort!

[michael-s-molina]

LGTM. Thank you for the improvements!

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.