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): lookml - add support for includes, extends, view_name i… #7428

Merged
merged 4 commits into from
Feb 24, 2023

Conversation

shirshanka
Copy link
Contributor

…n explore

Fixes a few issues with discovering views when navigating explores in the lookml source.

  • view_name is used for the name of the view when present
  • explores that live in included files (and their includes) are now included in the traversal
  • extends directives in explores are handled

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

@github-actions github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Feb 24, 2023
) -> "LookerExplore":
view_names = set()

view_names: Set[str] = set()
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

joins = None
# always add the explore's name or the name from the from clause as the view on which this explore is built
view_names.add(dict.get("from", dict.get("name")))
assert "name" in dict, "Explore doesn't have a name field, this isn't allowed"
Copy link
Contributor

Choose a reason for hiding this comment

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

asking to learn: if someone made a lookml like this, why isn't it allowed?

Copy link
Contributor

Choose a reason for hiding this comment

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

would this lookml also not be accepted by looker or is a datahub specific requirement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the lkml parser will populate the name field in the dict based on the explore definition, that is the contract.
So, it's not about having a name field inside the explore definition... more like parsing the explore will populate the name field.

e.g.

explore: events {
  ... explore def
}

will result in a dictionary with the name key set to events

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see, so we'd be looking at a malformed explore then if this wasn't parsed

looker_viewfile_loader,
view_name,
reporter,
upstream_views: List[ProjectInclude] = []
Copy link
Contributor

Choose a reason for hiding this comment

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

woo!

upstream_views.append(
ProjectInclude(project=info[0].project, include=view_name)
)
if extends:
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

reporter.report_warning(
path, f"Failed to load {included_file} due to {e}"
)
# continue in this case, as it might be better to load and resolve whatever we can
Copy link
Contributor

Choose a reason for hiding this comment

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

yes, agreed!

@@ -5,6 +5,7 @@ include: "bar.view.lkml"
include: "nested/*"
include: "liquid.view.lkml"
include: "ability.view.lkml"
include: "dataset_owners.explore.lkml"
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for adding a test

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants