Skip to content

update hash_token to CDM 5.1#122

Open
nihr43 wants to merge 1 commit into
masterfrom
5868_c7r2
Open

update hash_token to CDM 5.1#122
nihr43 wants to merge 1 commit into
masterfrom
5868_c7r2

Conversation

@nihr43

@nihr43 nihr43 commented Mar 5, 2020

Copy link
Copy Markdown

This table wasn't created as datavant expects.

sqlalchemy.exc.DatabaseError: (cx_Oracle.DatabaseError) ORA-00904: "Token_16": invalid identifier

@nihr43
nihr43 requested review from dckc and schandaka March 5, 2020 22:19
@nihr43 nihr43 changed the title resolve ivalid identifiers for datavant resolve invalid identifiers for datavant Mar 6, 2020
@dckc

dckc commented Mar 6, 2020

Copy link
Copy Markdown

Please cite the source of the names in a comment.

my Sep 10 comment still applies:

Did [we] get this code from somewhere? If so, please cite the source.

Keep in mind from our code review checklist:

  • Are external design constraints clear?
    • For example, is it clear which names were made up and which come from elsewhere?
  • If we're using anyone else's work, is it clearly licensed and acknowledged?

@dckc

dckc commented Mar 6, 2020

Copy link
Copy Markdown

perhaps it's clear enough in context that the names come from the CDM v5 spec. But since we didn't get it right the first time, I'm inclined to insist on a citation (title, date, url).

@nihr43

nihr43 commented Mar 6, 2020

Copy link
Copy Markdown
Author

hm, looks like datavant doesn't follow the cdm v5 spec.
these came from trial and error with the tool. I will invesigate if i can instruct it to use the correct column names

@dckc

dckc commented Mar 6, 2020

Copy link
Copy Markdown

There's a newer spec, which has, for example, TOKEN_16:

The Data-Driven materials include some spreadsheets that are sometimes handy.

@nihr43 nihr43 changed the title resolve invalid identifiers for datavant update hash_token to CDM 5.1 Mar 6, 2020
@nihr43

nihr43 commented Mar 6, 2020

Copy link
Copy Markdown
Author

cited doc source on table spec

@dckc dckc left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

looks good aside from pocket lint

@nihr43

nihr43 commented Mar 18, 2020

Copy link
Copy Markdown
Author

@schandaka , I noticed these columns were somehow present in c8.. schema. Did i update the wrong script here?

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