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

Fix segfault and errors on 2-sided sorted views when data window is invalid #1153

Merged
merged 3 commits into from Aug 26, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Aug 13, 2020

Retrieving data from a View can be controlled via the start_row/end_row and start_col/end_col arguments to to_json, which are parsed in a similar fashion to Python's range - inclusive of start and stopping at end - 1.

Invalid data windows - where start > end or start == end - are handled by returning empty data. However, in the special case of two-sided sorted views, it did not correctly handle invalid data windows while constructing a column indices vector, and would segfault or throw a ValueError.

This does not raise an issue with the viewer/plugins - the data window requested by perspective-viewer will be valid, even if the viewport is extremely small. However, this interferes with a Websocket testing suite I'm currently building, which randomly generates data windows with the expectation that they are handled correctly by Perspective.

Because there are many situations where a data window can be technically invalid, such as requesting a to_json on a view with 0 rows, or a view with 0 columns (such as the one created when generating autocomplete results for string filtering), I think it's better to have consistent behavior rather than throwing an error.

Changes

  • Fixes segfault and ValueError when data windows are invalid for 2-sided sorted views
  • Adds tests for all view contexts to check the behavior for invalid data windows are consistent

@timkpaine timkpaine added C++ enhancement Feature requests or improvements Python labels Aug 14, 2020
@sc1f sc1f added bug Concrete, reproducible bugs and removed enhancement Feature requests or improvements labels Aug 14, 2020
Copy link
Member

@texodus texodus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

I think I disagree that this is the desired behavior, since these windows are indisputably "invalid" in the whole-program-correctness, in that negative or zero sized windows are bad API calls from the consumer, and we should inform them as such with a constructive error. However, given the reality of our current perspective-viewer integration, this seems the change-of-least-impact for now.

@texodus
Copy link
Member

texodus commented Aug 26, 2020

Thanks for the PR!

@texodus texodus merged commit bc97557 into master Aug 26, 2020
@texodus texodus deleted the data-window branch August 26, 2020 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs C++ Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants