Skip to content

Unified data access interface#352

Merged
texodus merged 4 commits intomasterfrom
data_accessor
Dec 28, 2018
Merged

Unified data access interface#352
texodus merged 4 commits intomasterfrom
data_accessor

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

Development

Successfully merging this pull request may close these issues.

2 participants