-
Notifications
You must be signed in to change notification settings - Fork 84
Derived columns #367
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
Merged
Merged
Derived columns #367
Conversation
This file contains hidden or 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
just pushed up a new commit that catches runtime errors and returns them in the function output (although it seems like i need to update tests!). will put up a corresponding monorepo PR shortly! update: monorepo PR here |
libbey-observable
approved these changes
May 11, 2023
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.
Part of https://github.com/observablehq/observablehq/issues/11623
Description
This PR adds support for derived columns in the
__table
function. In this first version, we won't support derived columns for database sources.Notable changes:
applyTypes
function. We now run this twice, first on the source dataset, and second on the derived dataset, before merging the two. Type inference/coercion must be done separately on the derived dataset because it may depend on values in the source dataset already being coerced..fullSchema
property to the return value of__table
. This property contains the schema information for all columns in the dataset, regardless of whether or not they are selected (.schema
only contains the schema info for selected columns). In https://github.com/observablehq/observablehq/pull/11214, we switch to using the cell value to get the table schema, because derived columns aren't available on the original dataset and their types may be dynamic, so we need to look at their evaluated runtime values. We look at.fullSchema
when fetching the table schema so that we always have type information for the full set of columns, so that users can e.g. reselect a deselected column in the Columns menu.Review notes
I would love some feedback on the
.fullSchema
change! I'm not sure if it's the best way to make all the column types available, and I'm also not sure if I'm missing any major pitfalls with switching to use the cell value to fetch the table schema, instead of looking at the original data source as we do today. The main pitfall I experienced when testing is that, if there's an error in a derived formula, we no longer have a table schema available because the cell throws an error. I addressed that in the monorepo PR by adding a fallback that goes back to using the loaded data source + an approximation of the derived columns schema, which I think is the best we can do in that case. But perhaps there are other issues with this approach that I'm missing...