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

Fixes #1159 - deltas are now generated on first update for 0-sided contexts #1164

Merged
merged 7 commits into from
Aug 26, 2020

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Aug 24, 2020

Bugfix

#1159 reported an issue where row deltas were empty in tables after their first update from schema. This was due to the notify calls for first and subsequent updates being on different code paths (with different invocations of notify), and the line that added the primary key to be tracked as a changed row was missing. This PR fixes the issue, and adds a litany of tests in Javascript and Python.

Additionally, this PR fixes a previously overlooked issue - though we enable delta calculation based on whether they are used in on_update or not, 0-sided contexts would always calculate step and row deltas regardless of whether they were enabled or not. This seemed to be a default - get_deltas_enabled would always return true, and set_deltas_enabled would be a no-op for zero sided contexts.

Because there is no public API to get a delta without going through on_update and enabling deltas, and considering that row deltas are used far more widely than cell deltas (and cell deltas aren't even implemented in Python), it seems to make sense for 0-sided contexts to have the same behavior as 1/2 sided contexts, which respect get_deltas_enabled.

A next step might be differentiating cell/row delta enablement inside the context, which means that a callback that requests a row delta would not also enable cell delta calculation. This would be an internal API, as the cell/row delta methods are, and might bring some performance improvements.

This PR also adds benchmarking for deltas across different contexts, which means we can use it to test out actual performance over different versions.

Testing

Tests added for JS and Python, for multiple context types, computed columns, indexed, etc.

Screenshots (if appropriate)

Screen Shot 2020-08-24 at 3 22 18 PM

Checklist

  • I have read the CONTRIBUTING.md and followed its Guidelines
  • I have linted my code locally, following the project's code style
  • I have tested my changes locally, and changes pass on the Azure Pipelines CI.

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 this is the ideal implementation/test ratio of about 1/100 :)

@texodus texodus merged commit 1ee6766 into master Aug 26, 2020
@texodus texodus deleted the fix-remote-view branch August 26, 2020 14:36
@texodus texodus added bug Concrete, reproducible bugs enhancement Feature requests or improvements labels Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Concrete, reproducible bugs enhancement Feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants