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

fix(ingest): map all LookML dimension types to corresponding avro types #2972

Merged
merged 5 commits into from
Aug 2, 2021
Merged

fix(ingest): map all LookML dimension types to corresponding avro types #2972

merged 5 commits into from
Aug 2, 2021

Conversation

jameslamb
Copy link
Contributor

I noticed that when ingesting metadata for LookML files using the current state of master in this repo, I was getting warnings like the following:

The type 'date_month' is not recognized for field type, setting as NullTypeClass.
The type 'duration_hour' is not recognized for field type, setting as NullTypeClass.

I believe that's because some valid dimension types in LookML haven't been added to the mapping in datahub's LookML ingestion code. You can see the full list of dimension types at https://docs.looker.com/reference/field-reference/dimension-type-reference.

This pull request extends the work that was done in #2950 to map LookML dimension types to the corresponding avro types.

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)

Notes for Reviewers

I wasn't really sure what the right choice was for types like date_day_of_week_index (which is e.g. 0 on Sunday, 1 on Monday). I don't understand the implication of treating data like that as EnumTypeClass vs. NumberTypeClass. I expect reviewers will have some suggested changes to the types I chose here.

Thanks for your time and consideration!

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@shirshanka shirshanka merged commit fd20522 into datahub-project:master Aug 2, 2021
@jameslamb jameslamb deleted the fix/lookml-types branch August 2, 2021 18:21
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.

4 participants