Deprecate observation notice, optional fields, sql writing guide #280
Conversation
| nav_order: 9 | ||
| nav_order: 10 |
There was a problem hiding this comment.
This hard-coded nav order system we have does feel less graceful than I would like, but 🤷
There was a problem hiding this comment.
yeah it's not ideal, but we're not dealing with Gravity's Rainbow here or anything
| @@ -59,6 +59,38 @@ You can see which encounters were ignored as incomplete by examining the | |||
| Usually, you can resolve this by running the ETL process again for the encounters, | |||
| making sure to include all associated resources. | |||
|
|
|||
| ## Optional fields | |||
There was a problem hiding this comment.
Oof, this is reasonable documentation to include, but so easy to get out of date. And it's not like there's a single spot in the source where you could add a comment like "if you add a field, add it to the docs too"...
Though... maybe you could add a note like that to the five or so resource tables we support now?
But regardless, we can be vigilant.
There was a problem hiding this comment.
i'll add a checkbox item to the PR template - that remains surprisingly effective.
| @@ -368,6 +400,7 @@ making sure to include all associated resources. | |||
| | Column | Type |Description| | |||
| |------------|-------|-----------| | |||
| |id |varchar| | | |||
| |row |bigint | | | |||
There was a problem hiding this comment.
Hmmm... interesting.
I think these got removed by me, because that's what a generate-md did for me, when I pointed the Library at the BCH Cerner data. You also shouldn't have core__medication in your db anymore. I think you should re-run the core study on whatever data source you used (or use the BCH Cerner data)
There was a problem hiding this comment.
yeah I actually made some updates in one of the other PRs, i should regen this anyway.
There was a problem hiding this comment.
i am still seeing this after regen.
I've been running this against a synthetic dataset, which might have different structure? and/or may be out of date?
I'm going to leave this as is for now rather than rebuilding the cerner table before we release breaking changes, but I'm going to add a staging DB at some point that we can use for stuff like this.
docs/writing-sql.md
Outdated
|
|
||
| FHIR dates/times come across the wires as ISO 8061-formatted strings. | ||
| To get them into actual timestamps, use the `from_is08061_timestamp` function as | ||
| part of your query, and case to either `date` or `timestamp`.If you want to get a |
There was a problem hiding this comment.
nit: "case to" is a bit jargony, which might appropriate for a detailed SQL doc, but still - maybe "convert to"? Or use backticks to indicate you mean case as a method.
nit: missing a space between the period and "If"
There was a problem hiding this comment.
yeah lemme do CASE, since SQL is always yelling at you.
docs/writing-sql.md
Outdated
| date_trunc('day', date(from_iso8601_timestamp(dr."context"."period"."start"))) | ||
| AS author_day, |
There was a problem hiding this comment.
That might be an odd example, right - does date_trunc('day', date_value) actually do anything? Maybe truncate to month?
| FROM documentreference AS dr | ||
| ``` | ||
|
|
||
| ## Arrays |
There was a problem hiding this comment.
This section sounds vaguely familiar to me. Do we have SQL writing guidelines somewhere else too?
There was a problem hiding this comment.
are you thinking of the ticket you filed? 😄
there is some discussion of this in creating-sql-with-python.md, which is maybe a layer of abstraction above this doc.
docs/writing-sql.md
Outdated
| null values, since these are indistinguishable from the nulls created when cubing outputs. | ||
| Instead, | ||
| [coalesce](https://trino.io/docs/current/functions/conditional.html#coalesce) | ||
| fields to some static value, like 'None', if required. |
There was a problem hiding this comment.
Is it worth mentioning that the frontend handles values like cumulus__none well?
There was a problem hiding this comment.
probably, yeah
dogversioning
force-pushed
the
mg/docs_update
branch
from
baa4573 to
f3efef7
Compare
dogversioning
force-pushed
the
mg/docs_update
branch
from
f3efef7 to
45e7ece
Compare
dogversioning
force-pushed
the
mg/docs_update
branch
from
d0b9f4d to
29d8f2c
Compare
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
This addresses the following tickets via documentation:
#147 (along with smart-on-fhir/cumulus#35)
#214
#216
Checklist
docs/) needs to be updated