feat(ingestion/looker): Add view file-path as option in view_naming_pattern config #8713

siddiquebagwan-gslab · 2023-08-24T06:42:01Z

No description provided.

mayurinehate · 2023-08-29T11:19:06Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

-        )
+        n_mapping: NamingPatternMapping = self.get_mapping(config)
+        # / is not urn friendly
+        if n_mapping.file_path is not None:  # to silent the lint


n_mapping is not used anywhere else. Can we move this file_path specific handling inside get_mapping ?

Separately, maybe create a function named preprocess_file_path. Some other suggestions for this preprocessing:

remove .lkml and/or .view.lkml extensions

handle path for imported projects correctly (refer test_looker.py::test_looker_ingest_external_project_view for example of imported project tests)

looks like tests are not updated for this. Can you please update golden files ?

mayurinehate · 2023-08-29T14:21:35Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+    ) -> List[LookmlModelExploreField]:
+        return list(fields) if fields is not None else []
+
+    lkml_fields.extend(empty_list(explore.fields.dimensions))


Suggested change

lkml_fields.extend(empty_list(explore.fields.dimensions))

lkml_fields.extend(list(explore.fields.dimensions) or [])

same for below two statements. can remove nested function empty_list. It is already ensured earlier in code flow that explore.fields is not None.

empty_list will avoid empty list creation logic three places in the function.
empty_list is handling Not None for fields which are inside explore.fields (i.e. dimensions, measures, and parameters)

in this case, I prefer the or [] syntax over a new function

also, nesting functions like this is generally an antipattern

It is avoiding duplicates of or [] logic at three statement and more readable

mayurinehate · 2023-08-29T14:33:10Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+                file_path: str = (
+                    cast(str, self.upstream_views_file_path[view_ref.include])
+                    if self.upstream_views_file_path[view_ref.include] is not None
+                    else ViewFieldType.UNKNOWN.value


should we create a new unknown variable for this ? ViewFieldType.UNKNOWN feels misplaced here.

mayurinehate · 2023-08-29T14:35:13Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_config.py

@@ -81,6 +81,7 @@ class NamingPatternMapping:
    project: str
    model: str
    name: str
+    file_path: Optional[str]


file_path shows up as an option in explore_naming_pattern and explore_browse_pattern as well whereas we don't need them there. See - https://docs-website-1dask8vf5-acryldata.vercel.app/docs/next/generated/ingestion/sources/looker

Probably create a subclass ViewNamingPatternMapping with file_path:str and remove it from parent class ?

not able to see file_path in docs against view naming pattern and view_browse_path, can you please check ?

It is doc generation issue, need help from @hsheth2

mayurinehate · 2023-08-29T14:36:22Z

metadata-ingestion/src/datahub/ingestion/source/looker/lookml_source.py

+            logger.debug(
+                f"base_folder_path({base_folder_path}) and absolute_file_path({absolute_file_path}) not matching"
+            )
+            return ViewFieldType.UNKNOWN.value


same comment as new unknown variable.

Added new enum ViewFieldValue to address this

…iquebagwan-gslab/datahub-fork into master+ing-160-incomplete-urn-issue

mayurinehate · 2023-09-04T10:27:50Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+
+        str_to_replace: Dict[str, str] = {
+            f"imported_projects/{self.project_name}/": "",
+            "/": "_",  # / is not urn friendly


I personally prefer . over _ for readability. Also _ are common in folder names. Might lead to urn conflicts. e.g. if folder names are like a_b / a_c.view.lkml and a / b_a / c.view.lkml

Need accuracy check for imported_projects handling.
@hsheth2 - can you please review and confirm ?

Agree - let's do / -> .

mayurinehate · 2023-09-04T10:31:01Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+        }
+
+        for remove in str_to_remove:
+            new_file_path = new_file_path.rstrip(remove)


can't use rstrip here. It works on individual characters and not complete string.

hsheth2 · 2023-09-05T17:58:05Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+
+        str_to_replace: Dict[str, str] = {
+            f"imported_projects/{self.project_name}/": "",
+            "/": "_",  # / is not urn friendly


Agree - let's do / -> .

hsheth2 · 2023-09-05T17:58:41Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+
+        upstream_views_file_path[view_name] = file_path
+
+    logger.debug("Exit")


remove these debug lines

hsheth2 · 2023-09-05T18:02:06Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+    return None
+
+
+def create_upstream_views_file_path_map(


we shouldn't do this now, but I think it will make sense to more cleanly separate the looker and lookml code paths in the future e.g. this stuff really should live in looker_source.py, not looker common

Agree. I think it is happening because from_api is implemented in common

hsheth2 · 2023-09-05T18:03:24Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+    ) -> List[LookmlModelExploreField]:
+        return list(fields) if fields is not None else []
+
+    lkml_fields.extend(empty_list(explore.fields.dimensions))


in this case, I prefer the or [] syntax over a new function

also, nesting functions like this is generally an antipattern

hsheth2 · 2023-09-05T18:05:20Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

@@ -457,6 +571,9 @@ class LookerExplore:
    upstream_views: Optional[
        List[ProjectInclude]
    ] = None  # captures the view name(s) this explore is derived from
+    upstream_views_file_path: Dict[str, Optional[str]] = dataclasses_field(


why is this Optional[str]? it looks like it should be str since every view has a file path

Please go through function create_upstream_views_file_path_map and get_view_file_path . The function is looking for view-name on lkml_fields if view-name is found then return view-file-path othrewise return None

hsheth2 · 2023-09-05T18:06:24Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

+                # set file_path to ViewFieldType.UNKNOWN if file_path is not available to keep backward compatibility
+                # if we raise error on file_path equal to None then existing test-cases will fail as mock data doesn't have required attributes.
+                file_path: str = (
+                    cast(str, self.upstream_views_file_path[view_ref.include])


why do we need this cast

because self.upstream_views_file_path[view_ref.include] is optional, None check is there but still lint was raising error

hsheth2 · 2023-09-05T18:06:45Z

metadata-ingestion/src/datahub/ingestion/source/looker/looker_common.py

@@ -791,12 +925,20 @@ def _to_metadata_events(  # noqa: C901
            upstreams = []
            observed_lineage_ts = datetime.datetime.now(tz=datetime.timezone.utc)
            for view_ref in sorted(self.upstream_views):
+                # set file_path to ViewFieldType.UNKNOWN if file_path is not available to keep backward compatibility
+                # if we raise error on file_path equal to None then existing test-cases will fail as mock data doesn't have required attributes.


why can't we update the test mock data?

I am not sure whether mock data is written as per production scenario. The fields are marked as Optional in looker sdk, so to avoid any surprises in production I preferred to keep existing test as is

siddiquebagwan-gslab added 6 commits August 14, 2023 13:19

WIP

08e5ba3

wip

7bb90d3

Merge branch 'master' into master+ing-160-incomplete-urn-issue

de8fddd

Merge branch 'master' into master+ing-160-incomplete-urn-issue

851a711

add file_path support in looker

243cc6e

Merge branch 'master' into master+ing-160-incomplete-urn-issue

554300b

github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Aug 24, 2023

vercel bot deployed to Preview August 24, 2023 06:54 View deployment

siddiquebagwan-gslab added 2 commits August 24, 2023 13:23

add documentation

55ff427

Add documentation

c6bef18

vercel bot deployed to Preview August 24, 2023 08:31 View deployment

siddiquebagwan-gslab added 2 commits August 24, 2023 17:20

looker test-case for file_path

134cf6d

test cases added

10bea8a

asikowitz requested review from hsheth2 and mayurinehate August 24, 2023 17:07

siddiquebagwan-gslab added 2 commits August 25, 2023 20:39

lineage verify

ffccf9a

Merge branch 'master' into master+ing-160-incomplete-urn-issue

00269af

vercel bot deployed to Preview August 25, 2023 15:25 View deployment

Merge branch 'master' into master+ing-160-incomplete-urn-issue

ba313df

vercel bot deployed to Preview August 28, 2023 10:13 View deployment

mayurinehate requested changes Aug 29, 2023

View reviewed changes

siddiquebagwan-gslab added 3 commits August 30, 2023 17:31

Merge branch 'master' into master+ing-160-incomplete-urn-issue

c3c8cfb

address review comments

c3fdab9

Merge branch 'master+ing-160-incomplete-urn-issue' of github.com:sidd…

4a8e2dc

…iquebagwan-gslab/datahub-fork into master+ing-160-incomplete-urn-issue

vercel bot deployed to Preview August 31, 2023 11:57 View deployment

Merge branch 'master' into master+ing-160-incomplete-urn-issue

f10c4b6

vercel bot deployed to Preview September 4, 2023 03:40 View deployment

mayurinehate reviewed Sep 4, 2023

View reviewed changes

hsheth2 reviewed Sep 5, 2023

View reviewed changes

Merge branch 'master' into master+ing-160-incomplete-urn-issue

8ef9026

vercel bot deployed to Preview September 7, 2023 11:51 View deployment

review comments

c239f5d

vercel bot deployed to Preview September 7, 2023 13:46 View deployment

siddiquebagwan-gslab added 2 commits September 8, 2023 17:12

Merge branch 'master' into master+ing-160-incomplete-urn-issue

61f7899

address review comments

acd56c3

vercel bot deployed to Preview September 8, 2023 13:35 View deployment

hsheth2 approved these changes Sep 8, 2023

View reviewed changes

hsheth2 added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Sep 8, 2023

anshbansal merged commit 95b2d43 into datahub-project:master Sep 11, 2023
57 checks passed

	lkml_fields.extend(empty_list(explore.fields.dimensions))
	lkml_fields.extend(list(explore.fields.dimensions) or [])


		upstream_views_file_path[view_name] = file_path

		logger.debug("Exit")

feat(ingestion/looker): Add view file-path as option in view_naming_pattern config #8713

feat(ingestion/looker): Add view file-path as option in view_naming_pattern config #8713

Conversation

siddiquebagwan-gslab commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayurinehate Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddiquebagwan-gslab Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddiquebagwan-gslab Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

mayurinehate Aug 29, 2023 •

edited

Loading

siddiquebagwan-gslab Sep 8, 2023 •

edited

Loading

siddiquebagwan-gslab Sep 8, 2023 •

edited

Loading