Skip to content
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

chore(ingest/tableau): miscellaneous cleanup refractor #8417

Merged
merged 7 commits into from
Jul 27, 2023

Conversation

mayurinehate
Copy link
Collaborator

Checklist

  • The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
  • Links to related issues (if applicable)
  • Tests for the changes have been added/updated (if applicable)
  • Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
  • For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

Copy link
Collaborator

@asikowitz asikowitz left a comment

Choose a reason for hiding this comment

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

All nice changes, thanks for doing this! I think I see one functional code change.

Also, these tableau goldens are absurd lol. Would be nice to get some stripped down ones that are actually reviewable

schema = table.get(tableau_constant.SCHEMA, "")
table_name = table.get(tableau_constant.NAME, "")
full_name = table.get(tableau_constant.FULL_NAME, "")
schema = table.get(tableau_constant.SCHEMA) or ""
Copy link
Collaborator

@asikowitz asikowitz Jul 14, 2023

Choose a reason for hiding this comment

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

Is it possible table[NAME] == None? Just curious on the reason for these changes, overall I'm happy with them

tableau_constant.EXTRACT_LAST_UPDATE_TIME, ""
)
or "",
tableau_constant.TYPE: datasource.get(tableau_constant.TYPE_NAME, ""),
Copy link
Collaborator

Choose a reason for hiding this comment

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

We're fetching key TYPE_NAME but only TYPE in the new code

Copy link
Collaborator Author

@mayurinehate mayurinehate Jul 18, 2023

Choose a reason for hiding this comment

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

nice catch.

So it fetches the key named "__typename" from dictionary and adds it to custom props with key "type". I believe, it is okay to keep __typename as key in custom dict OR remove this key entirely , as the type of datasource is captured in subtype.

@asikowitz
Copy link
Collaborator

Merging as this shouldn't affect smoke tests and it seems like the failures were due to the tests taking too long... gonna try one more time first

@asikowitz asikowitz added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Jul 25, 2023
@anshbansal anshbansal merged commit b9060db into datahub-project:master Jul 27, 2023
40 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ingestion PR or Issue related to the ingestion of metadata merge-pending-ci A PR that has passed review and should be merged once CI is green.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants