deephaven / deephaven-core

Deephaven Community Core
Other
257 stars 80 forks source link

Any `Comparable` should be sortable via the UI #6109

Open chipkent opened 2 months ago

chipkent commented 2 months ago

LocalDate and LocalTime columns can not be sorted via the UI.

Use this code to create a table. Neither column can be sorted in the table.

from deephaven import empty_table

t = empty_table(10).update([
    "DT = '2023-01-01' + i * 'P1d'",
    "LT = '12:34:56'.plus(i * 'PT1s')",
    ])

m = t.meta_table

Note: a Comparable groovy test case is In the comments below.

nbauernfeind commented 1 month ago

This isn't reproducible on main. I can sort both of the columns on t.

Not only does the code look right:

    private static boolean isDataTypeSortable(final Class<?> dataType) {
        return dataType.isPrimitive() || Comparable.class.isAssignableFrom(dataType);
    }

The debugger says these columns are sortable:

Screenshot 2024-10-01 at 2 19 21 PM
niloc132 commented 1 month ago

@chipkent can you confirm, or give us more details (build/version)?

chipkent commented 1 month ago

I think this is a UI thing. I can still reproduce it on the demo system. Create a table and then try to sort either column with a right-click menu. Note that it is grayed-out below.

image
chipkent commented 1 month ago

The demo system is 0.36.1.

niloc132 commented 1 month ago

Appears to be confirmed that this is fixed in 0.37, closing.

nbauernfeind commented 1 month ago

This was fixed for LocalDate and LocalTime in #6063 which was enabled by the jsapi refactoring.

This actually suggests that we won't mark previewed columns as sortable even if they are. That's the real bug here.

This is the right repro:

class T implements Comparable<T> {
    private int id;

    public T(int id) {
        this.id = id;
    }

    @Override
    public int compareTo(T o) {
        if (id < o.id) return -1
        if (id > o.id) return 1
        return 0
    }
}

import io.deephaven.engine.context.ExecutionContext

ExecutionContext.getContext().getQueryLibrary().importClass(T.class)

t = emptyTable(10).update("T = new T(i)")
nbauernfeind commented 1 month ago

@niloc132 says that there is another ticket to push the applyPreviewColumns call later to right before subscribing.

He believes that it will fix this issue and other related issues to previewed columns.

I'll add the ticket reference here once found.