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

Unified data access interface #352

Merged
merged 4 commits into from
Dec 28, 2018
Merged

Unified data access interface #352

merged 4 commits into from
Dec 28, 2018

Conversation

sc1f
Copy link
Contributor

@sc1f sc1f commented Dec 26, 2018

  • Move data ingestion and metadata inference into C++
  • Create a unified API for accessing data regardless of underlying format (columnar vs. row-based)
  • Ported inference functions return native C++ types

new_pdata.push(Object.assign({}, pdata, chunk));
}
pdata = new_pdata;
} else {
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit tricky - unsure if we will need this again in the future. It was oroginalyl added to prevent vecFromJSArray() calls in C++ from blowing up the wasm heap, but data accessor no longer calls this per column.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do call vecFromJSArray() for each load, update, and delete operation instead of per-column, so there is still the danger of blowing up the WASM heap even with this change.

I was working on a version of the accessor that did not rely on the JS side passing in column names and data types, as we already have that information stored on the C++ side (thus avoiding the vecFromJSArray() casting), but I ran into some non-deterministic memory access out of bounds issues.

Theoretically it's possible to access the already-created gnode in make_table, grab the schema from there, and then grab the names and types. Actually doing it, however, creates a bunch of errors that I haven't been able to nail down yet.

@texodus
Copy link
Member

texodus commented Dec 28, 2018

Thanks! This is a great addition, very nice work!

@texodus texodus merged commit 58a8bca into master Dec 28, 2018
@texodus texodus deleted the data_accessor branch December 28, 2018 23:35
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.

2 participants