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

JS ChartData type doesn't handle shifts correctly #2435

Closed
Tracked by #5927
devinrsmith opened this issue May 18, 2022 · 7 comments · Fixed by #6076
Closed
Tracked by #5927

JS ChartData type doesn't handle shifts correctly #2435

devinrsmith opened this issue May 18, 2022 · 7 comments · Fixed by #6076
Assignees
Labels
bug Something isn't working
Milestone

Comments

@devinrsmith
Copy link
Member

Not the prettiest query, but it seems to get the job done.

Create the following ticking table, and then use chart builder to create a bar chart with TimeBin on x-axis and Count on y-axis, w/ sync'd state. Add a quick-filter on M is equal to 8. The chart will likely tick correctly for a little bit, but then some it will likely stop ticking and will show incorrect data.

t = timeTable("00:00:00.001")
  .updateView("TimeBin=lowerBin(Timestamp, 1000000000)", "I=i", "K=i%3+i%5+i%7+i%11", "M=K%3+K%5+K%7+K%11")
  .sort("M")
  .countBy("Count", "TimeBin", "M")
  .sort("M")

image

A similar demonstration has been achieved w/ pie chart.

@devinrsmith devinrsmith added bug Something isn't working triage labels May 18, 2022
@devinrsmith
Copy link
Member Author

Note: hold off on investigation until I can prove it's not due to #2405.

@devinrsmith
Copy link
Member Author

I've verified this bug did exist before #2405. That said, it's probably best to test and fix this against a previous commit to main, 727c9ed (until #2405 is verified fixed).

@vbabich vbabich removed the triage label May 25, 2022
@mofojed
Copy link
Member

mofojed commented May 25, 2022

Python snippet:

from deephaven import time_table

tt = time_table("00:00:00.001")\
.update_view(["TimeBin=lowerBin(Timestamp, 1000000000)", "I=i", "K=i%3+i%5+i%7+i%11", "M=K%3+K%5+K%7+K%11"])\
.sort("M")\
.count_by("Count", ["TimeBin", "M"])\
.sort("M")

While testing I'm hitting an index error in ChartData:

assert nextAdded.getLast() < nextIndex;// the whole range should be there if any is

Maybe @niloc132 would want to know? Digging a bit more before passing off

@mofojed
Copy link
Member

mofojed commented May 25, 2022

Looks like the value of nextAdded.last and nextIndex are the same:
image

@mofojed
Copy link
Member

mofojed commented May 25, 2022

This also happens on Enterprise. Here's debug logging with some failure updates - in this case we're looking for 1073741864:

16:40:43.794 VM12549:1 MJB handleSnapshot RangeSet{sortedRanges=[Range{first=1073740932, last=1073740944}, Range{first=1073741009, last=1073741072}, Range{first=1073741137, last=1073741200}, Range{first=1073741265, last=1073741328}, Range{first=1073741393, last=1073741456}, Range{first=1073741521, last=1073741584}, Range{first=1073741649, last=1073741712}, Range{first=1073741777, last=1073741989}, Range{first=1073742008, last=1073742094}, Range{first=1073742136, last=1073742245}, Range{first=1073742264, last=1073742350}, Range{first=1073742392, last=1073742423}]}
16:40:44.175 VM12551:1 MJB handleSnapshot RangeSet{sortedRanges=[Range{first=1073740932, last=1073740944}, Range{first=1073741009, last=1073741072}, Range{first=1073741137, last=1073741200}, Range{first=1073741265, last=1073741328}, Range{first=1073741393, last=1073741456}, Range{first=1073741521, last=1073741584}, Range{first=1073741649, last=1073741712}, Range{first=1073741777, last=1073741989}, Range{first=1073742008, last=1073742094}, Range{first=1073742136, last=1073742245}, Range{first=1073742264, last=1073742350}, Range{first=1073742392, last=1073742423}]}
16:40:45.181 VM12556:1 MJB handleData deltaUpdates added RangeSet{sortedRanges=[Range{first=1073741053, last=1073741053}, Range{first=1073741184, last=1073741184}, Range{first=1073741315, last=1073741315}, Range{first=1073741446, last=1073741446}, Range{first=1073741577, last=1073741577}, Range{first=1073741708, last=1073741708}, Range{first=1073741839, last=1073741839}, Range{first=1073741905, last=1073741906}, Range{first=1073741974, last=1073741974}, Range{first=1073742057, last=1073742057}, Range{first=1073742165, last=1073742165}, Range{first=1073742233, last=1073742233}, Range{first=1073742316, last=1073742316}, Range{first=1073742424, last=1073742424}]} 
removed RangeSet{sortedRanges=[]} 
shift (13) [Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2), Array(2)]0: (2) ['Range{first=1073741009, last=1073741053}', -1]1: (2) ['Range{first=1073741137, last=1073741184}', -1]2: (2) ['Range{first=1073741265, last=1073741315}', -1]3: (2) ['Range{first=1073741393, last=1073741446}', -1]4: (2) ['Range{first=1073741521, last=1073741577}', -1]5: (2) ['Range{first=1073741649, last=1073741708}', -1]6: (2) ['Range{first=1073741777, last=1073741839}', -1]7: (2) ['Range{first=1073741905, last=1073741971}', 2]8: (2) ['Range{first=1073741972, last=1073741989}', 3]9: (2) ['Range{first=1073742057, last=1073742094}', 1]10: (2) ['Range{first=1073742165, last=1073742231}', 1]0: "Range{first=1073742165, last=1073742231}"1: 1length: 2[[Prototype]]: Array(0)11: (2) ['Range{first=1073742232, last=1073742245}', 2]12: (2) ['Range{first=1073742316, last=1073742350}', 1]length: 13[[Prototype]]: Array(0) 
serializedModifications (3) ['RangeSet{sortedRanges=[]}', 'RangeSet{sortedRanges=[]}', 'RangeSet{sortedRanges=[Range{first=1073741183, las…2232}, Range{first=1073742423, last=1073742423}]}']0: "RangeSet{sortedRanges=[]}"1: "RangeSet{sortedRanges=[]}"2: "RangeSet{sortedRanges=[Range{first=1073741183, last=1073741183}, Range{first=1073741314, last=1073741314}, Range{first=1073741445, last=1073741445}, Range{first=1073741576, last=1073741576}, Range{first=1073741707, last=1073741707}, Range{first=1073741838, last=1073741838}, Range{first=1073741973, last=1073741973}, Range{first=1073742056, last=1073742056}, Range{first=1073742164, last=1073742164}, Range{first=1073742232, last=1073742232}, Range{first=1073742423, last=1073742423}]}"length: 3[[Prototype]]: Array(0)

@niloc132 helped with the investigation.

Stack trace

  | overrideMethod | @ | react_devtools_backend.js:4026
  | $lambda$3 | @ | dh-core.js:7809
  | onInvoke_20 | @ | dh-core.js:9576
  | lambda | @ | dh-core.js:150
  | $fireEvent_0 | @ | dh-core.js:7783
  | $fireEvent | @ | dh-core.js:7773
  | $notifyUpdates | @ | dh-core.js:21177
  | $handleDelta | @ | dh-core.js:21121
  | $handleDelta_0 | @ | dh-core.js:21768
  | $lambda$74 | @ | dh-core.js:10342
  | apply_25 | @ | dh-core.js:11875
  | lambda | @ | dh-core.js:150
  | $lambda$15_1 | @ | dh-core.js:28885
  | run_46 | @ | dh-core.js:29544
  | lambda | @ | dh-core.js:150
  | onInvoke_175 | @ | dh-core.js:28210
  | lambda | @ | dh-core.js:150
  | Promise.then (async) |   |  
  | runLater | @ | dh-core.js:28101
  | $onRunning | @ | dh-core.js:29018
  | $onRunning_0 | @ | dh-core.js:29034
  | $lambda$28_3 | @ | dh-core.js:28923
  | apply_201 | @ | dh-core.js:29688
  | lambda | @ | dh-core.js:150
  | $lambda$26 | @ | dh-core.js:28914
  | apply_199 | @ | dh-core.js:29659
  | lambda | @ | dh-core.js:150
  | lambda$1_7 | @ | dh-core.js:27843
  | onInvoke_174 | @ | dh-core.js:27928
  | lambda | @ | dh-core.js:150
  | $forActiveSubscriptions | @ | dh-core.js:28721
  | $handleDelta_1 | @ | dh-core.js:28761
  | lambda$29 | @ | dh-core.js:14180
  | accept_19 | @ | dh-core.js:15130
  | $ifPresent | @ | dh-core.js:36468
  | $lambda$28_0 | @ | dh-core.js:13624
  | run_19 | @ | dh-core.js:15121
  | lambda | @ | dh-core.js:150
  | onInvoke_175 | @ | dh-core.js:28210
  | lambda | @ | dh-core.js:150
  | Promise.then (async) |   |  
  | runLater | @ | dh-core.js:28101
  | $appendAndMaybeFlush | @ | dh-core.js:14610
  | $apply_3 | @ | dh-core.js:14625
  | apply_46 | @ | dh-core.js:14662
  | lambda | @ | dh-core.js:150
  | (anonymous) | @ | dh-internal.js:1
  | (anonymous) | @ | dh-internal.js:1
  | (anonymous) | @ | dh-internal.js:1
  | e.rawOnMessage | @ | dh-internal.js:1
  | (anonymous) | @ | dh-internal.js:1
  | e.onTransportChunk | @ | dh-internal.js:1
  | t.onmessage | @ | dh-internal.js:1

@mofojed mofojed transferred this issue from deephaven/web-client-ui May 25, 2022
@mofojed mofojed assigned niloc132 and unassigned mofojed May 25, 2022
@niloc132
Copy link
Member

niloc132 commented May 25, 2022

The underlying issue is SubscriptionTableData doesn't propagate shifts to consumers, like TableData, so the data is broken if shifts appear. Apparently this has always been broken, since shifts were introduced.

The easiest fix to consume by JS would be to normalized shifts into add/modify/removes in the existing event, but the most efficient would be to propagate them to TableData to avoid unnecessary data operations. Adding a "shiftObliviousListener" sort of helper here might be best, so that TableData can get the most efficient code, and JS consumers can opt out of handling shifts.

Any fix made here should cherry-pick cleanly to enterprise.

@devinrsmith
Copy link
Member Author

I wonder if we should do an audit of listeners to see if there are any other obvious spots that are missing shift handling :s

@devinrsmith devinrsmith added this to the Jun 2022 milestone May 25, 2022
@niloc132 niloc132 modified the milestones: Jun 2022, Backlog Jul 22, 2022
@niloc132 niloc132 changed the title Chart builder update bug JS ChartData type doesn't handle shifts correctly Sep 17, 2024
niloc132 added a commit to niloc132/deephaven-core that referenced this issue Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants