feat(ingestion/lookml): resolve access notation for LookML Constant #12277

sagar-salvi-apptware · 2025-01-06T16:15:38Z

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

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

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

sgomezvillamor

LGTM

My only suggestion would be to refactor the logic in resolve_lookml_parameter into a separate class. Then, the different warning scenarios can be easily unit tested.

hsheth2 · 2025-01-09T21:42:51Z

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

+        def replace_parameters(match):
+            key = match.group(1)
+            # Check if it's a misplaced liquid variable
+            if key in self.source_config.liquid_variable:


we should check the lookml_parameter field first

this was not done - lookml_constants should take precedence over the manifest file

this is old code in newer commits i have removed it already

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

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

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

codecov · 2025-01-10T07:09:52Z

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ngestion/source/looker/looker_template_language.py	80.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
...ahub/ingestion/source/looker/looker_dataclasses.py	`83.48% <ø> (ø)`
...ahub/ingestion/source/looker/looker_file_loader.py	`79.59% <ø> (-0.41%)`	⬇️
...c/datahub/ingestion/source/looker/lookml_config.py	`93.00% <ø> (-0.07%)`	⬇️
...c/datahub/ingestion/source/looker/lookml_source.py	`79.28% <ø> (+0.21%)`	⬆️
...ngestion/source/looker/looker_template_language.py	`93.70% <80.00%> (+1.45%)`	⬆️

... and 1515 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 842c8f9...f1daf12. Read the comment docs.

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

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

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

...a-ingestion/tests/integration/lookml/vv-lineage-and-liquid-templates/activity_logs.view.lkml

metadata-ingestion/docs/sources/looker/lookml_post.md

hsheth2 · 2025-01-16T00:55:54Z

metadata-ingestion/tests/integration/lookml/test_lookml.py

@@ -869,11 +869,15 @@ def test_view_to_view_lineage_and_liquid_template(pytestconfig, tmp_path, mock_t
            "dev_database_prefix": "employee",
            "dev_schema_prefix": "public",
        },
+        "_dev_database_attributes": {
+            "dev_database_prefix": "customer",
+        },
        "dw_eff_dt_date": {
            "_is_selected": True,
        },
        "source_region": "ap-south-1",
    }


we need some unit tests focused specifically on LookmlConstantTransformer

where are the unit tests? we have full integration tests for this, but we should really have a more targeted unit test that creates a LookmlConstantTransformer and tests its behavior in isolation

hi @hsheth2 this is old code new commits will have new test for lookml constant.
0b54bb6

hsheth2

Please don't resolve comments if things aren't actually resolved. If you're unsure if you've done what was asked for, leave the comment unresolved and I can take another look.

metadata-ingestion/docs/sources/looker/looker_recipe.yml

hsheth2 · 2025-01-17T18:57:18Z

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

+        def replace_parameters(match):
+            key = match.group(1)
+            # Check if it's a misplaced liquid variable
+            if key in self.source_config.liquid_variable:


this was not done - lookml_constants should take precedence over the manifest file

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

hsheth2 · 2025-01-17T19:01:36Z

metadata-ingestion/tests/integration/lookml/test_lookml.py

@@ -869,11 +869,15 @@ def test_view_to_view_lineage_and_liquid_template(pytestconfig, tmp_path, mock_t
            "dev_database_prefix": "employee",
            "dev_schema_prefix": "public",
        },
+        "_dev_database_attributes": {
+            "dev_database_prefix": "customer",
+        },
        "dw_eff_dt_date": {
            "_is_selected": True,
        },
        "source_region": "ap-south-1",
    }


where are the unit tests? we have full integration tests for this, but we should really have a more targeted unit test that creates a LookmlConstantTransformer and tests its behavior in isolation

sagar-salvi-apptware · 2025-01-18T13:58:35Z

Hi @hsheth2,
In the newer commits all changes has been cover except liquid_variable -> liquid_variables as it was existing field dont want to rename it but i ll rename this also

lookml parameter support

fa4f5f2

sagar-salvi-apptware changed the title ~~lookml parameter support~~ feat(ingestion/lookml): resolve access notation for LookML parameters Jan 6, 2025

github-actions bot added the ingestion PR or Issue related to the ingestion of metadata label Jan 6, 2025

vercel bot deployed to Preview January 6, 2025 16:33 View deployment

hsheth2 mentioned this pull request Jan 7, 2025

feat(ingestion/lookml): resolve access notation for LookML parameters #12172

Closed

sagar-salvi-apptware added 2 commits January 8, 2025 16:40

fix: pr comment

865b5ff

test: updated golden files

36f746f

sagar-salvi-apptware marked this pull request as ready for review January 8, 2025 11:14

sagar-salvi-apptware force-pushed the fix/ING-784-lookml-parameter branch from e6de550 to 4ef052f Compare January 8, 2025 11:18

fix: minor change

4a9ec88

sagar-salvi-apptware force-pushed the fix/ING-784-lookml-parameter branch from 4ef052f to 4a9ec88 Compare January 8, 2025 11:20

datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Jan 8, 2025

vercel bot deployed to Preview January 8, 2025 11:41 View deployment

sgomezvillamor reviewed Jan 8, 2025

View reviewed changes

metadata-ingestion/src/datahub/ingestion/source/looker/looker_template_language.py Outdated Show resolved Hide resolved

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 8, 2025

sgomezvillamor reviewed Jan 8, 2025

View reviewed changes

metadata-ingestion/src/datahub/ingestion/source/looker/lookml_config.py Outdated Show resolved Hide resolved

fix: pr comments

750cf33

sgomezvillamor reviewed Jan 8, 2025

View reviewed changes

vercel bot deployed to Preview January 8, 2025 17:08 View deployment

hsheth2 reviewed Jan 9, 2025

View reviewed changes

fix: pr comment

4e7bc94

sagar-salvi-apptware force-pushed the fix/ING-784-lookml-parameter branch from 710eff2 to 4e7bc94 Compare January 10, 2025 06:23

vercel bot deployed to Preview January 10, 2025 06:49 View deployment

datahub-cyborg bot removed the pending-submitter-response Issue/request has been reviewed but requires a response from the submitter label Jan 10, 2025

sagar-salvi-apptware added 2 commits January 15, 2025 16:09

fix: refactor changes to lookml constant

ae605d9

test: updated test for lookml constant

a7cc9d5

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jan 15, 2025

vercel bot had a problem deploying to Preview January 15, 2025 10:47 Failure

fix: fixed lint issue

46316ca

vercel bot deployed to Preview January 15, 2025 11:47 View deployment

hsheth2 requested changes Jan 16, 2025

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 16, 2025

sagar-salvi-apptware changed the title ~~feat(ingestion/lookml): resolve access notation for LookML parameters~~ feat(ingestion/lookml): resolve access notation for LookML Constant Jan 16, 2025

sagar-salvi-apptware added 2 commits January 16, 2025 15:23

fix: pr comments

2c16fc6

test: updated test for lookml constant

0b54bb6

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jan 16, 2025

vercel bot deployed to Preview January 16, 2025 10:30 View deployment

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 17, 2025

fix: pr comment

f08b827

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jan 17, 2025

vercel bot deployed to Preview January 17, 2025 08:18 View deployment

hsheth2 requested changes Jan 17, 2025

View reviewed changes

datahub-cyborg bot added pending-submitter-response Issue/request has been reviewed but requires a response from the submitter and removed needs-review Label for PRs that need review from a maintainer. labels Jan 17, 2025

datahub-cyborg bot added needs-review Label for PRs that need review from a maintainer. and removed pending-submitter-response Issue/request has been reviewed but requires a response from the submitter labels Jan 18, 2025

fix: rename liquid_variable -> liquid_variables

f1daf12

vercel bot deployed to Preview January 18, 2025 15:14 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingestion/lookml): resolve access notation for LookML Constant #12277

feat(ingestion/lookml): resolve access notation for LookML Constant #12277

sagar-salvi-apptware commented Jan 6, 2025

sgomezvillamor left a comment

hsheth2 Jan 9, 2025

hsheth2 Jan 17, 2025

sagar-salvi-apptware Jan 18, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

hsheth2 Jan 16, 2025

hsheth2 Jan 17, 2025

sagar-salvi-apptware Jan 18, 2025 •

edited

Loading

hsheth2 left a comment

hsheth2 Jan 17, 2025

hsheth2 Jan 17, 2025

sagar-salvi-apptware commented Jan 18, 2025

feat(ingestion/lookml): resolve access notation for LookML Constant #12277

Are you sure you want to change the base?

feat(ingestion/lookml): resolve access notation for LookML Constant #12277

Conversation

sagar-salvi-apptware commented Jan 6, 2025

Checklist

sgomezvillamor left a comment

Choose a reason for hiding this comment

hsheth2 Jan 9, 2025

Choose a reason for hiding this comment

hsheth2 Jan 17, 2025

Choose a reason for hiding this comment

sagar-salvi-apptware Jan 18, 2025

Choose a reason for hiding this comment

codecov bot commented Jan 10, 2025 • edited Loading

Codecov Report

hsheth2 Jan 16, 2025

Choose a reason for hiding this comment

hsheth2 Jan 17, 2025

Choose a reason for hiding this comment

sagar-salvi-apptware Jan 18, 2025 • edited Loading

Choose a reason for hiding this comment

hsheth2 left a comment

Choose a reason for hiding this comment

hsheth2 Jan 17, 2025

Choose a reason for hiding this comment

hsheth2 Jan 17, 2025

Choose a reason for hiding this comment

sagar-salvi-apptware commented Jan 18, 2025

codecov bot commented Jan 10, 2025 •

edited

Loading

sagar-salvi-apptware Jan 18, 2025 •

edited

Loading