Skip to content
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

DH-17242: PartitionedTable display widget does not render properly when there are non-key, non-constituent columns #2066

Closed
rcaudy opened this issue Jun 10, 2024 · 7 comments · Fixed by #2110
Assignees
Labels
blocker bug Something isn't working
Milestone

Comments

@rcaudy
Copy link
Member

rcaudy commented Jun 10, 2024

Description

When a PartitionedTable is created that has extra columns (not just key columns and the constituent columns), the display widget fails to render the partition filter widget or any table data. A box at the bottom of the component informs the user that it is "Reloading partition".

Steps to reproduce

You can reproduce with this script in a Groovy console:

tt = timeTable("PT00:00:01") // Make a TimeTable
it = tt.update("A=ii%2==0", "B=k%10") // Add some columns
pt = it.partitionBy("A", "B") // Partition by those columns
ptt = pt.table() // Grab the underlying table
pttu = ptt.update("C=A ? B : -B") // Derive a new column
pt2 = io.deephaven.engine.table.PartitionedTableFactory.of(pttu, List.of("A","B"), true, "__CONSTITUENT__", pt.constituentDefinition(), false) // Make a new PartitionedTable with the same key columns

Another way to reproduce is with a partitionedAggBy:

pt = it.partitionedAggBy([AggLast("Last=Timestamp")], false, null, "A", "B")

In both cases an extra column in the underlying table seems to be confusing the UI. This is an important, permitted use case for PartitionedTables.

Expected results

I expect a partition filtering widget to show as normal. Ideally, though, we'd have some better way to display this, since showing the extra columns of the partitioned table would be valuable. I'm not certain that PartitionedTable and PartitionAwareSourceTable should be displayed in the same way.

Additional details and attachments

DH-17242

Versions

  • Engine Version: latest
  • Web UI Version: ...
  • Java Version: ...
  • Barrage Version: ...
  • OS: ...
  • Browser: ...
  • Docker: ...
@rcaudy rcaudy added bug Something isn't working triage Issue requires triage labels Jun 10, 2024
@dsmmcken
Copy link
Contributor

Ideally, though, we'd have some better way to display this, since showing the extra columns of the partitioned table would be valuable. I'm not certain that PartitionedTable and PartitionAwareSourceTable should be displayed in the same way.

Can you elaborate on what you are thinking?

@mattrunyon mattrunyon changed the title PartitionedTable display widget does not render properly when there are non-key, non-constituent columns DH-17242: PartitionedTable display widget does not render properly when there are non-key, non-constituent columns Jun 11, 2024
@vbabich vbabich added the question Further information is requested label Jun 12, 2024
@vbabich vbabich self-assigned this Jun 12, 2024
@vbabich vbabich added this to the June 2024 milestone Jun 12, 2024
@vbabich
Copy link
Collaborator

vbabich commented Jun 12, 2024

There's a ticket for differentiating the UI for PartitionedTable and PartitionAwareSourceTable - #2049

@vbabich vbabich removed the triage Issue requires triage label Jun 12, 2024
@rcaudy
Copy link
Member Author

rcaudy commented Jun 13, 2024

Ideally, though, we'd have some better way to display this, since showing the extra columns of the partitioned table would be valuable. I'm not certain that PartitionedTable and PartitionAwareSourceTable should be displayed in the same way.

Can you elaborate on what you are thinking?

If I were designing a "perfect" PartitionedTable widget, I might want the following:

  1. The underlying table() displayed by default. Color-coding or some other visual indicator identifying the key columns and the constituent columns.
  2. Key column selectors and a merge() button available, as today, for selecting a single constituent to display or merging the entire partitioned table.
  3. The ability to select a constituent to display from the table, rather than requiring the user to fiddle with the dropdowns.
  4. The ability to filter or sort the underlying table being displayed (this might call for more of the PartitionedTable Java API to be exposed over gRPC).

@dsmmcken
Copy link
Contributor

dsmmcken commented Jun 13, 2024

Same as example in ticket just as python, except for one line I am not sure on the equivalent

from deephaven import agg
from deephaven import time_table

tt = time_table("PT00:00:01") # Make a TimeTable
it = tt.update(["A=ii%2==0", "B=k%10"]) # Add some columns
pt = it.partition_by(["A", "B"]) # Partition by those columns
ptt = pt.table # Grab the underlying table

pttu = ptt.update("C=A ? B : -B") # Derive a new column
# unsure how make this python
# pt2 = io.deephaven.engine.table.PartitionedTableFactory.of(pttu, List.of("A","B"), true, "__CONSTITUENT__", pt.constituentDefinition(), false) // Make a new PartitionedTable with the same key columns

# aggregate it
ptagg = it.partitioned_agg_by(
    aggs=[agg.last(cols=["Last = Timestamp"])], by=["A","B"]
)

@mofojed mofojed assigned AkshatJawne and unassigned vbabich Jun 17, 2024
@mofojed mofojed removed the question Further information is requested label Jun 17, 2024
@mofojed
Copy link
Member

mofojed commented Jun 17, 2024

@rcaudy can we get some clarification on point 1?

The underlying table() displayed by default. Color-coding or some other visual indicator identifying the key columns and the constituent columns.

Is there another case that would demonstrate this a little bit better? In the example above, there are the two key columns, and then there is a column _CONSTITUENT_ which looks automatically generated... you want to be able to see that?

We'll need to have a JS API added for this at least, as there is no way to get the underlying table for a partitioned table at the moment.

image

@rcaudy
Copy link
Member Author

rcaudy commented Jun 17, 2024

@rcaudy can we get some clarification on point 1?

The underlying table() displayed by default. Color-coding or some other visual indicator identifying the key columns and the constituent columns.

Is there another case that would demonstrate this a little bit better? In the example above, there are the two key columns, and then there is a column _CONSTITUENT_ which looks automatically generated... you want to be able to see that?

We'll need to have a JS API added for this at least, as there is no way to get the underlying table for a partitioned table at the moment.

image

I had a fairly extensive meeting with @dsmmcken and @rbasralian on Friday afternoon, and I don't want to recap that here unless there's a need for clarification. If you look at the partitionedAggBy example, you'll see more about what I mean about the importance of the underlying table, because of the additional non-key, non-constituent columns that might be present. __CONSTITUENT__ is an automatically-generated name, but users are also able to supply their own names; QueryTable in this case is the description of the table, and need not always be so boring.

@mofojed
Copy link
Member

mofojed commented Jun 19, 2024

I should have asked the previous question on #2079 instead of this ticket, let's keep this ticket focussed on just the fix.

Debugging a little bit, it looks like the JsPartitionedTable.keyColumns returns just two key columns, ['A', 'B']; however, the JsPartitionedTable.keyTable returns a table with 3 columns, ['A', 'B', 'Last'], which doesn't seem correct.

If you run the code on the Java side looking at the keys table on the ptagg object, it is returning a table with only two columns:

print(ptagg.key_columns) # outputs ['A', 'B']
print(ptagg.keys().columns) # outputs [Column(name='A', data_type=java.lang.Boolean, component_type=None, column_type=NORMAL), Column(name='B', data_type=long, component_type=None, column_type=NORMAL)]

So it seems correct there, so for some reason the Last column is being added in somewhere, and I don't think it should be.

@vbabich vbabich modified the milestones: June 2024, July 2024 Jul 9, 2024
mofojed pushed a commit to deephaven/deephaven-core that referenced this issue Jul 17, 2024
## Release notes
https://github.com/deephaven/web-client-ui/releases/tag/v0.86.0

### Features

* Add option to disable WebGL rendering
([#2134](deephaven/web-client-ui#2134))
([011eb33](deephaven/web-client-ui@011eb33))
* Core plugins refactor, XComponent framework
([#2150](deephaven/web-client-ui#2150))
([2571fad](deephaven/web-client-ui@2571fad))
* IrisGridTheme iconSize
([#2123](deephaven/web-client-ui#2123))
([58ee88d](deephaven/web-client-ui@58ee88d)),
closes [#885](deephaven/web-client-ui#885)
* Partitioned Table UI Enhancements
([#2110](deephaven/web-client-ui#2110))
([de5ce40](deephaven/web-client-ui@de5ce40)),
closes [#2079](deephaven/web-client-ui#2079)
[#2066](deephaven/web-client-ui#2066)
[#2103](deephaven/web-client-ui#2103)
[#2104](deephaven/web-client-ui#2104)
[#2105](deephaven/web-client-ui#2105)
[#2106](deephaven/web-client-ui#2106)
[#2107](deephaven/web-client-ui#2107)
[#2108](deephaven/web-client-ui#2108)
[#2109](deephaven/web-client-ui#2109)
[#2049](deephaven/web-client-ui#2049)
[#2120](deephaven/web-client-ui#2120)
[#1904](deephaven/web-client-ui#1904)


### Bug Fixes

* error when edited cell is out of grid viewport
([#2148](deephaven/web-client-ui#2148))
([3fccd43](deephaven/web-client-ui@3fccd43)),
closes [#2087](deephaven/web-client-ui#2087)

## Release notes
https://github.com/deephaven/web-client-ui/releases/tag/v0.85.2


### Bug Fixes

* Fix missing scrim background on LoadingOverlay
([#2098](deephaven/web-client-ui#2098))
([c9ed895](deephaven/web-client-ui@c9ed895))

## Release notes
https://github.com/deephaven/web-client-ui/releases/tag/v0.85.1

##
[0.85.1](deephaven/web-client-ui@v0.85.0...v0.85.1)
(2024-07-08)


### Bug Fixes

* re-export remaining types needed by dh ui from @react-types/shared
([#2132](deephaven/web-client-ui#2132))
([2119a61](deephaven/web-client-ui@2119a61))



## Release notes
https://github.com/deephaven/web-client-ui/releases/tag/v0.85.0

### Features

* ComboBox - @deephaven/jsapi-components
([#2077](deephaven/web-client-ui#2077))
([115e057](deephaven/web-client-ui@115e057)),
closes [#2074](deephaven/web-client-ui#2074)


### Bug Fixes

* Allow ComboBox to accept the FocusableRef for ref
([#2121](deephaven/web-client-ui#2121))
([8fe9bad](deephaven/web-client-ui@8fe9bad))
* Ref was not being passed through for Picker
([#2122](deephaven/web-client-ui#2122))
([a11e2ce](deephaven/web-client-ui@a11e2ce))

Co-authored-by: deephaven-internal <66694643+deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment