Add and export getSchema function #363
Conversation
|
hm on second thought, maybe we should handle type assertions outside of |
src/table.js
Outdated
| schema = inferSchema(source, isQueryResultSetColumns(columns) ? columns : undefined); | ||
| return {schema, shouldCoerce: true}; | ||
| } | ||
| return {schema, shouldCoerce: false}; |
There was a problem hiding this comment.
Maybe we should call this “inferred” instead of “shouldCoerce”? Because “inferred” makes a factual observation about how the schema was derived, whereas “shouldCoerce” implies that the caller should do something with the returned schema—and perhaps the caller wants to do something else?
src/table.js
Outdated
| const schemaInfo = getSchema(source); | ||
| let {columns} = source; | ||
| let {schema, inferred} = schemaInfo; |
There was a problem hiding this comment.
Minor simplification:
| const schemaInfo = getSchema(source); | |
| let {columns} = source; | |
| let {schema, inferred} = schemaInfo; | |
| let {columns} = source; | |
| let {schema, inferred} = getSchema(source); |
Also: I don’t think we should fix it now, but there’s a latent bug below where where we’re assuming that if source.columns is truthy, it’s a valid QueryResultSetColumns. That’s because this function is trying to preserve the same shape of output as the input; so, if the input had a source.columns, we return a result.columns. I don’t think we need to do that; we could just always return a result.schema instead, now that we’re inferring a schema as needed. But again, reasonable to do that as a separate change to minimize risk since it’s not directly related to this PR’s objective.
There was a problem hiding this comment.
hm yeah, now that .schema is always defined, do we even need to return .columns from this function or can we just remove it? or would that be a breaking change, since we export __table from stdlib?
There was a problem hiding this comment.
Right, I meant I think that __table can always return .schema (and never .columns). I don’t think that would be breaking… 🤷
This PR adds a
getSchemafunction, building on a suggestion from Mike. This function conditionally infers a schema depending on whether or not a valid one exists on the source, combines the schema with any saved type assertions, and determines whether or not the source data needs to be coerced.We will use
getSchemain both__tableand in the table schema tasks in the worker (I'll update https://github.com/observablehq/observablehq/pull/11496 to use this function), and thereby unify the two paths that we currently have for type inference.I also added a missing unit test for type assertion in
__table, as well as some tests forgetSchema.