-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Cleaner API through the Table abstraction, prepare for Python API #642
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
please see the other PR and revert the python changes again |
its fine to just |
@timkpaine Python is on the same state as master, although it'll fail the build because the bindings have not been updated. |
Rebased on top of latest master. |
add as(), call() raw pointers broken
…scripten.cpp and binding document binding.h and clean up emscripten.cpp make val.h header only, allow python tests to temporarily fail
Looks good, thanks for the PR! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR overhauls the internal workings of Perspective.js and the core engine by wrapping the pool and gnode in the
Table
class, which manages operations on the pool/gnode.Previously, the C++ engine's notion of a "table" was an internal structure that wasn't fully expressive of what the Javascript notion of "table" was, notably the management of the pool/gnode structures that were essential to the JS conception of the table. The new abstraction introduced in this PR unifies the notions of a
Table
across the core and JS library, which means further API development should be clearer in terms of requirements on what a table is, does, etc.Additionally, this PR prepares for the Python API by making the binding surface to the core engine more accessible and easily shared. Through the
t_val
andt_data_accessor
class, we aim to increase code reusability between the Emscripten and the Python bindings.When work starts on the Python API, we'll be able to write our custom implementations of
t_val
andt_data_accessor
that uses Pybind, and simply plug it in to the existing methods defined inbinding.h
.@timkpaine: I built this refactor on top of the Python changes I had made, and dropping the commit and rebasing it is super gnarly because all my commits are on top of it. The old Python code is still in Git, so replacing it for now (especially since we're going to rewrite Python starting in the next week) should be fine.
Some notable/specific changes:
t_table
becomest_data_table
t_val
andt_data_accessor
are typedef'd toemscripten::val
in the WASM buildcolumn_metadata
from table API as it was not being usedMemory semantics around the new Table structure should be equivalent to the current implementation of table/pool/gnode—preliminary benchmarks show no significant impact on performance for better or worse.