OpenLineage: Rename the name and namespace along with URI #1306

rajaths010494 · 2022-11-24T09:34:20Z

Describe the bug

There have been new conventions introduced: Naming.md#local-file-system
I believe sqlite counts as local file, therefore you could adjust naming convention in astro as well.

A word of explanation: both IP and port are mandatory. It means that if port is unknown (e. g. if file exists in Airflow's local file system only) you should default to some value. In one of OL extractors it is defaulted to paramiko.config.SSH_PORT, in most cases it's equal to 22.

Reference: #1142 (comment)

Add openlineage_dataset_uri() method to create URI as per each database.

closes #1281

JDarDagran

LGTM

python-sdk/src/astro/databases/sqlite.py

codecov · 2022-11-24T09:57:25Z

Codecov Report

Base: 95.00% // Head: 94.99% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (b200223) compared to base (0c64f48).
Patch coverage: 94.44% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1306      +/-   ##
==========================================
- Coverage   95.00%   94.99%   -0.02%     
==========================================
  Files          71       71              
  Lines        3304     3315      +11     
  Branches      381      381              
==========================================
+ Hits         3139     3149      +10     
- Misses        101      102       +1     
  Partials       64       64

Impacted Files	Coverage Δ
python-sdk/src/astro/sql/operators/append.py	`95.65% <ø> (-0.19%)`	⬇️
...thon-sdk/src/astro/sql/operators/base_decorator.py	`95.00% <ø> (ø)`
python-sdk/src/astro/sql/operators/dataframe.py	`100.00% <ø> (ø)`
python-sdk/src/astro/sql/operators/export_file.py	`93.87% <ø> (-0.13%)`	⬇️
python-sdk/src/astro/sql/operators/load_file.py	`96.62% <ø> (-0.04%)`	⬇️
python-sdk/src/astro/sql/operators/merge.py	`95.83% <ø> (-0.17%)`	⬇️
python-sdk/src/astro/databases/base.py	`95.71% <50.00%> (-0.44%)`	⬇️
python-sdk/src/astro/databases/aws/redshift.py	`94.02% <100.00%> (+0.09%)`	⬆️
python-sdk/src/astro/databases/google/bigquery.py	`94.62% <100.00%> (+0.05%)`	⬆️
python-sdk/src/astro/databases/postgres.py	`96.62% <100.00%> (+0.07%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

rajaths010494 · 2022-11-24T11:58:11Z

Attaching a screenshot of the URI for reference on Marquez UI.

sunank200

LGTM

sunank200 · 2022-11-25T09:38:15Z

@rajaths010494 @JDarDagran is the dataSource uri in the correct format as aspected?

JDarDagran · 2022-11-25T10:05:12Z

Actually why do you need this format:

f"{self.input_data.openlineage_dataset_namespace()}"
f"://{self.input_data.openlineage_dataset_name()}"

with two slashes before dataset name? The path should be absolute so I'd rather see URI as file://172.26.0.2:22/tmp/sqlite.db.top_animation.

This spec, 3.3. Path explains more.

sunank200

Actually why do you need this format:
f"{self.input_data.openlineage_dataset_namespace()}"
f"://{self.input_data.openlineage_dataset_name()}"
with two slashes before dataset name? The path should be absolute so I'd rather see URI as file://172.26.0.2:22/tmp/sqlite.db.top_animation.

This spec, 3.3. Path explains more.

@rajaths010494 we should do this change across.

rajaths010494 · 2022-11-25T11:06:37Z

Actually why do you need this format:
f"{self.input_data.openlineage_dataset_namespace()}"
f"://{self.input_data.openlineage_dataset_name()}"
with two slashes before dataset name? The path should be absolute so I'd rather see URI as file://172.26.0.2:22/tmp/sqlite.db.top_animation.
This spec, 3.3. Path explains more.
@rajaths010494 we should do this change across.

Yeah saw i will make the necessary changes

rajaths010494 · 2022-11-25T11:13:51Z

Actually why do you need this format:
f"{self.input_data.openlineage_dataset_namespace()}"
f"://{self.input_data.openlineage_dataset_name()}"
with two slashes before dataset name? The path should be absolute so I'd rather see URI as file://172.26.0.2:22/tmp/sqlite.db.top_animation.
This spec, 3.3. Path explains more.
@rajaths010494 we should do this change across.
Yeah saw i will make the necessary changes

Does this look fine. @JDarDagran @sunank200 . If yes i will make the necessary changes.

JDarDagran · 2022-11-25T11:16:26Z

Actually why do you need this format:
f"{self.input_data.openlineage_dataset_namespace()}"
f"://{self.input_data.openlineage_dataset_name()}"
with two slashes before dataset name? The path should be absolute so I'd rather see URI as file://172.26.0.2:22/tmp/sqlite.db.top_animation.
This spec, 3.3. Path explains more.
@rajaths010494 we should do this change across.
Yeah saw i will make the necessary changes
Does this look fine. @JDarDagran @sunank200 . If yes i will make the necessary changes.

👍

…/astronomer/astro-sdk into rename-localfilesystem-namespace

sunank200

@rajaths010494 Change looks good to me.But lets add the test for this change.

sunank200 · 2022-11-29T10:33:09Z

We should create a method for URI for each database and use that. I will do that change and push it

sunank200 · 2022-11-29T11:42:45Z

Ran and tested the example DAGs for operators on Marquez.

sunank200 · 2022-11-29T11:48:13Z

"https://user-images.githubusercontent.com/8670962/204521134-a276f0ff-7e3d-434f-a4c9-ccf11aac53d7.png">

rajaths010494 · 2022-11-29T11:48:21Z

LGTM

Describe the bug - There have been new conventions introduced: [Naming.md#local-file-system](https://github.com/OpenLineage/OpenLineage/blob/main/spec/Naming.md#local-file-system) I believe sqlite counts as local file, therefore you could adjust naming convention in astro as well. A word of explanation: both IP and port are mandatory. It means that if port is unknown (e. g. if file exists in Airflow's local file system only) you should default to some value. In one of OL extractors it is defaulted to paramiko.config.SSH_PORT, in most cases it's equal to 22. Reference: #1142 (comment) - Add openlineage_dataset_uri() method to create URI as per each database. closes #1281 Co-authored-by: Ankit Chaurasia <8670962+sunank200@users.noreply.github.com> (cherry picked from commit b725430)

JDarDagran approved these changes Nov 24, 2022

View reviewed changes

python-sdk/src/astro/databases/sqlite.py Outdated Show resolved Hide resolved

rajaths010494 added 3 commits November 24, 2022 17:28

Fix OL namespace of sqlite

27c3d8e

Address review comments

1469900

remove extra /

c603c1d

rajaths010494 force-pushed the rename-localfilesystem-namespace branch from c82cd84 to c603c1d Compare November 24, 2022 11:58

rajaths010494 marked this pull request as ready for review November 24, 2022 15:54

rajaths010494 requested review from dimberman, tatiana, utkarsharma2, sunank200, pankajastro, pankajkoti and feluelle as code owners November 24, 2022 15:54

sunank200 approved these changes Nov 25, 2022

View reviewed changes

sunank200 requested changes Nov 25, 2022

View reviewed changes

rajaths010494 added 5 commits November 28, 2022 11:05

appropriate OL uri

9cced6c

Fix OL namespace of sqlite

a3f6772

Address review comments

5a60a2f

remove extra /

4760bff

appropriate OL uri

b8a8296

rajaths010494 force-pushed the rename-localfilesystem-namespace branch from 9cced6c to b8a8296 Compare November 28, 2022 05:36

rajaths010494 added 2 commits November 28, 2022 11:06

Merge branch 'rename-localfilesystem-namespace' of https://github.com…

f679207

…/astronomer/astro-sdk into rename-localfilesystem-namespace

fix test failure postgres

cd5420e

sunank200 requested changes Nov 28, 2022

View reviewed changes

kaxil changed the title ~~Rename the name and namespace for sqlite as per new convention introduced~~ OpenLineage: Rename the name and namespace for sqlite Nov 28, 2022

Add openlineage_dataset_uri for table along with test

206144d

sunank200 changed the title ~~OpenLineage: Rename the name and namespace for sqlite~~ OpenLineage: Rename the name and namespace along with URI Nov 29, 2022

Merge branch 'main' into rename-localfilesystem-namespace

a9a3dff

sunank200 approved these changes Nov 29, 2022

View reviewed changes

phanikumv approved these changes Nov 29, 2022

View reviewed changes

sunank200 mentioned this pull request Nov 29, 2022

Support Google Drive as file location #1322

Merged

1 task

Merge branch 'main' into rename-localfilesystem-namespace

b200223

sunank200 merged commit b725430 into main Nov 29, 2022

sunank200 deleted the rename-localfilesystem-namespace branch November 29, 2022 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenLineage: Rename the name and namespace along with URI #1306

OpenLineage: Rename the name and namespace along with URI #1306

rajaths010494 commented Nov 24, 2022 •

edited by sunank200

Loading

JDarDagran left a comment

codecov bot commented Nov 24, 2022 •

edited

Loading

rajaths010494 commented Nov 24, 2022

sunank200 left a comment

sunank200 commented Nov 25, 2022

JDarDagran commented Nov 25, 2022 •

edited

Loading

sunank200 left a comment

rajaths010494 commented Nov 25, 2022

rajaths010494 commented Nov 25, 2022

JDarDagran commented Nov 25, 2022

sunank200 left a comment

sunank200 commented Nov 29, 2022

sunank200 commented Nov 29, 2022

sunank200 commented Nov 29, 2022

rajaths010494 commented Nov 29, 2022

OpenLineage: Rename the name and namespace along with URI #1306

OpenLineage: Rename the name and namespace along with URI #1306

Conversation

rajaths010494 commented Nov 24, 2022 • edited by sunank200 Loading

JDarDagran left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 24, 2022 • edited Loading

Codecov Report

rajaths010494 commented Nov 24, 2022

sunank200 left a comment

Choose a reason for hiding this comment

sunank200 commented Nov 25, 2022

JDarDagran commented Nov 25, 2022 • edited Loading

sunank200 left a comment

Choose a reason for hiding this comment

rajaths010494 commented Nov 25, 2022

rajaths010494 commented Nov 25, 2022

JDarDagran commented Nov 25, 2022

sunank200 left a comment

Choose a reason for hiding this comment

sunank200 commented Nov 29, 2022

sunank200 commented Nov 29, 2022

sunank200 commented Nov 29, 2022

rajaths010494 commented Nov 29, 2022

rajaths010494 commented Nov 24, 2022 •

edited by sunank200

Loading

codecov bot commented Nov 24, 2022 •

edited

Loading

JDarDagran commented Nov 25, 2022 •

edited

Loading