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 chart cell to handle empty data sets #20

Merged

Conversation

rliebling
Copy link
Contributor

@rliebling rliebling commented Sep 26, 2022

If you create a Chart smart cell while some of your bindings are for datasets with no results then the Chart cell fails to work. That is, you cannot choose any dataset in the "Data" input field. The problem is that the private function infer_types encounters an error because for the :rows case because it assumes there is at least one row.

The solution here is to explicitly check for whether the dataset has data. This does mean that a result set with no data won't be offered as a choice in the Chart smart cell. I could see instead having infer_types() return a list matching the length of the columns, but with either nil or "nominal" for each value so that the dataset would be allowed when empty.

@josevalim
Copy link
Contributor

Hi @rliebling! The third element is not guaranteed to be a list, so perhaps the alternative fix is the way to go? Thank you!

@rliebling rliebling force-pushed the fix_handling_empty_dataset branch 4 times, most recently from 211d864 to f4215aa Compare September 27, 2022 02:17
@rliebling
Copy link
Contributor Author

@josevalim Thanks for pointing that out! I updated the code, and the description of the PR. This feels clean, but I realized after that maybe one would still want to choose the empty dataset or at least you might get bug reports if they are not offered as choices. I'm fine changing to allow that, if you prefer. Just please let me know what infer_types should return for an empty dataset.

@josevalim
Copy link
Contributor

@rliebling ah, that's another great idea. We could show empty datasets but we rule out any dataset without columns, and that will remove silly false positives such as empty lists (or empty maps).

@jonatanklosko
Copy link
Member

Just please let me know what infer_types should return for an empty dataset.

A list of nils should work fine, as in Enum.map(columns, fn _ -> nil end) :)

@rliebling
Copy link
Contributor Author

Sounds good - thanks! will get to this in a couple days. I assume nobody else is urgently waiting on this so the delay is fine.

@rliebling rliebling force-pushed the fix_handling_empty_dataset branch 4 times, most recently from 343aa02 to f610644 Compare October 2, 2022 02:54
@rliebling
Copy link
Contributor Author

Ok - thinks this should be good now.

@jonatanklosko jonatanklosko changed the title fix scan_binding() handling empty data set in chart_cell.ex Fix chart cell to handle empty data sets Oct 2, 2022
Issue is `infer_types({:rows, _, _})` assumes that there is
at least 1 row so that it can `Enum.map` over the columns.

We will check in `columns_for` that the list of columns is
not empty.  If so, and the dataset is empty, then the new
implementation of `infer_types()` will return a list of
`nil`s to allow the scan to succeed but without inferred types.
@rliebling rliebling force-pushed the fix_handling_empty_dataset branch from f610644 to b445214 Compare October 2, 2022 13:46
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

Great, thank you!

@jonatanklosko jonatanklosko merged commit 42dc30a into livebook-dev:main Oct 2, 2022
@rliebling
Copy link
Contributor Author

Thank you both for your patience! You set a terrific example for the community (and open source) - when it undoubtedly would have been easier for you to just fix my initial attempts and commit it than to give me the feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants