Fix open lineage namespace for Sqlite as per OL team request #1142

sunank200 · 2022-10-27T06:01:02Z

Description

What is the current behavior?

The sqlite namespace is missing in OpenLineage naming convention. So we created the namespace by default. OL team has requested to change this namespace.
The Namespaces extracted from AstroSDK do not conform to the OpenLineage naming convention for sqlite. For example, /tmp/sqlite_default.db instead of e.g. sqllite://

closes: #1141

What is the new behavior?

Fix the namespace for sqlite
Fix the test for this

Does this introduce a breaking change?

No

Checklist

Created tests which fail without the change (if possible)
Extended the README / documentation, if necessary

…Lineage/blob/main/spec/Naming.md

codecov · 2022-10-27T06:08:04Z

Codecov Report

Base: 91.94% // Head: 93.82% // Increases project coverage by +1.87% 🎉

Coverage data is based on head (7e326f9) compared to base (496382c).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1142      +/-   ##
==========================================
+ Coverage   91.94%   93.82%   +1.87%     
==========================================
  Files          12       65      +53     
  Lines         472     3045    +2573     
  Branches       48      341     +293     
==========================================
+ Hits          434     2857    +2423     
- Misses         28      129     +101     
- Partials       10       59      +49

Impacted Files	Coverage Δ
python-sdk/src/astro/databases/sqlite.py	`96.42% <100.00%> (ø)`
sql-cli/sql_cli/__main__.py	`100.00% <0.00%> (ø)`
sql-cli/sql_cli/exceptions.py	`100.00% <0.00%> (ø)`
python-sdk/src/astro/sql/operators/load_file.py	`96.47% <0.00%> (ø)`
python-sdk/src/astro/databases/base.py	`97.00% <0.00%> (ø)`
python-sdk/src/astro/files/locations/http.py	`100.00% <0.00%> (ø)`
python-sdk/src/astro/databases/postgres.py	`96.29% <0.00%> (ø)`
python-sdk/src/astro/sql/operators/raw_sql.py	`89.65% <0.00%> (ø)`
python-sdk/src/astro/files/locations/__init__.py	`100.00% <0.00%> (ø)`
python-sdk/src/astro/files/locations/amazon/s3.py	`97.56% <0.00%> (ø)`
... and 47 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.

pankajastro

LGTM. Thanks, @sunank200 for fixing it.

pankajastro · 2022-10-27T06:22:53Z

python-sdk/tests/databases/test_base_database.py

@@ -18,6 +18,24 @@ class DatabaseSubclass(BaseDatabase):
    pass


+def test_openlineage_database_dataset_namespace():


just wondering does this test add some value?

It is just for code coverage. It was failing for coverage

I think this is a good point from @pankajastro, the test should check that the method returns what you expect.
Test coverage reports tell you where you're missing tests, coverage is not a goal in itself.
In this case, the test should check that the method returns a namespace that conforms with the spec.

I agree with @pankajastro and @julienledem here, this test doesn't add any value.

It is just for code coverage. It was failing for coverage

If we just need to do this for code coverage, we are doing it wrong :)

Sure. We already have a test which covers the scenarios: https://github.com/astronomer/astro-sdk/blob/main/python-sdk/tests/sql/test_table.py#L221

julienledem

Thank you @sunank200 for addressing the request.
Sorry for not giving my review before this got merged.
Please look at the few comments bellow.
I think you still need to tweak it before we can consider it done.
Best, Julien

julienledem · 2022-10-27T17:31:41Z

python-sdk/tests/databases/test_base_database.py

@@ -18,6 +18,24 @@ class DatabaseSubclass(BaseDatabase):
    pass


+def test_openlineage_database_dataset_namespace():


I think this is a good point from @pankajastro, the test should check that the method returns what you expect.
Test coverage reports tell you where you're missing tests, coverage is not a goal in itself.
In this case, the test should check that the method returns a namespace that conforms with the spec.

julienledem · 2022-10-27T17:33:34Z

python-sdk/tests/databases/test_base_database.py

+    """
+    db = DatabaseSubclass(conn_id="fake_conn_id")
+    with pytest.raises(NotImplementedError):
+        db.openlineage_dataset_name(table=BaseTable)


Similarly, here you want to mock the method being called to check that openlineage_dataset_name returns the expected value.

julienledem · 2022-10-27T17:33:46Z

python-sdk/tests/databases/test_base_database.py

+    with pytest.raises(NotImplementedError):
+        db.openlineage_dataset_name(table=BaseTable)
+
+
 def test_subclass_missing_not_implemented_methods_raise_exception():
    db = DatabaseSubclass(conn_id="fake_conn_id")
    with pytest.raises(NotImplementedError):


julienledem · 2022-10-27T17:52:26Z

python-sdk/tests/sql/test_table.py

-            "/tmp/sqlite.db",
+            Connection(conn_id="test_conn", conn_type="sqlite", host="tmp/sqlite.db"),
+            "tmp/sqlite.db.test_tb",
+            "sqlite://tmp/sqlite.db",


Two comments here:

The namespace should not repeat the dataset name ("tmp/sqlite.db.test_tb").

The tricky aspect of those local datasets that are private to the machine they are running on is we want the dataset ns+name to be unique. Otherwise if you run this on two different machines the lineage graph will assume that you are writing and reading the same dataset, which is not true. You need to make the namespace more unique by (for example) adding the machine name/IP in it.

I have added the PR for the change #1178. Please review

# Description ## What is the current behavior?  Fix the comments made on #1142  closes: #1179 ## What is the new behavior?  - Fix the namespace for sqlite as per comment on #1142 > Two comments here: > > - The namespace should not repeat the dataset name ("tmp/sqlite.db.test_tb"). >- The tricky aspect of those local datasets that are private to the machine they are running on is we want the dataset ns+name to be unique. Otherwise if you run this on two different machines the lineage graph will assume that you are writing and reading the same dataset, which is not true. You need to make the namespace more unique by (for example) adding the machine name/IP in it. - Fix the test for this ## Does this introduce a breaking change? No ### Checklist - [x] Created tests which fail without the change (if possible) - [x] Extended the README / documentation, if necessary Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

# Description ## What is the current behavior?  The sqlite namespace is missing in [OpenLineage naming convention](https://github.com/OpenLineage/OpenLineage/blob/main/spec/Naming.md). So we created the namespace by default. OL team has requested to change this namespace. The Namespaces extracted from AstroSDK do not conform to the [OpenLineage naming convention](https://github.com/OpenLineage/OpenLineage/blob/main/spec/Naming.md) for sqlite. For example, /tmp/sqlite_default.db instead of e.g. sqllite://  closes: #1141 ## What is the new behavior?  - Fix the namespace for sqlite - Fix the test for this ## Does this introduce a breaking change? No ### Checklist - [x] Created tests which fail without the change (if possible) - [x] Extended the README / documentation, if necessary

# Description ## What is the current behavior?  Fix the comments made on #1142  closes: #1179 ## What is the new behavior?  - Fix the namespace for sqlite as per comment on #1142 > Two comments here: > > - The namespace should not repeat the dataset name ("tmp/sqlite.db.test_tb"). >- The tricky aspect of those local datasets that are private to the machine they are running on is we want the dataset ns+name to be unique. Otherwise if you run this on two different machines the lineage graph will assume that you are writing and reading the same dataset, which is not true. You need to make the namespace more unique by (for example) adding the machine name/IP in it. - Fix the test for this ## Does this introduce a breaking change? No ### Checklist - [x] Created tests which fail without the change (if possible) - [x] Extended the README / documentation, if necessary Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

JDarDagran · 2022-11-21T07:22:53Z

Hey, 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.

sunank200 · 2022-11-22T09:29:20Z

Hey, 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.

@JDarDagran created the issue for this: #1281

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>

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)

Fix open lineage namespace as per https://github.com/OpenLineage/Open…

bf18a9c

…Lineage/blob/main/spec/Naming.md

sunank200 requested review from dimberman, tatiana, utkarsharma2, pankajastro, pankajkoti and feluelle as code owners October 27, 2022 06:01

sunank200 changed the title ~~Fix open lineage namespace as per https://github.com/OpenLineage/Open…~~ Fix open lineage namespace for Sqlite Oct 27, 2022

Add test to fix open lineage dataset name and namespace

a466804

sunank200 changed the title ~~Fix open lineage namespace for Sqlite~~ Fix open lineage namespace for Sqlite as per OL team request Oct 27, 2022

pankajastro approved these changes Oct 27, 2022

View reviewed changes

pankajastro reviewed Oct 27, 2022

View reviewed changes

Fix sqlite namespace

7e326f9

sunank200 self-assigned this Oct 27, 2022

sunank200 merged commit c2773d5 into main Oct 27, 2022

sunank200 deleted the rename-database-namespace branch October 27, 2022 06:54

julienledem reviewed Oct 27, 2022

View reviewed changes

conorbev mentioned this pull request Oct 27, 2022

Add sqllite to Naming doc OpenLineage/OpenLineage#1186

Open

This was referenced Nov 2, 2022

Change the namespace for open lineage #1178

Merged

Fix namespace as per OL team recommentation #1179

Closed

sunank200 mentioned this pull request Nov 22, 2022

Rename the name and namespace for sqlite as per new convention introduced #1281

Closed

rajaths010494 mentioned this pull request Nov 24, 2022

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix open lineage namespace for Sqlite as per OL team request #1142

Fix open lineage namespace for Sqlite as per OL team request #1142

sunank200 commented Oct 27, 2022 •

edited

Loading

codecov bot commented Oct 27, 2022 •

edited

Loading

pankajastro left a comment

pankajastro Oct 27, 2022

sunank200 Oct 27, 2022

julienledem Oct 27, 2022

kaxil Nov 1, 2022

sunank200 Nov 2, 2022

julienledem left a comment

julienledem Oct 27, 2022

julienledem Oct 27, 2022

julienledem Oct 27, 2022

julienledem Oct 27, 2022

sunank200 Nov 2, 2022

JDarDagran commented Nov 21, 2022

sunank200 commented Nov 22, 2022

		@@ -18,6 +18,24 @@ class DatabaseSubclass(BaseDatabase):
		pass


		def test_openlineage_database_dataset_namespace():

Fix open lineage namespace for Sqlite as per OL team request #1142

Fix open lineage namespace for Sqlite as per OL team request #1142

Conversation

sunank200 commented Oct 27, 2022 • edited Loading

Description

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Checklist

codecov bot commented Oct 27, 2022 • edited Loading

Codecov Report

pankajastro left a 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

julienledem left a 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

JDarDagran commented Nov 21, 2022

sunank200 commented Nov 22, 2022

sunank200 commented Oct 27, 2022 •

edited

Loading

codecov bot commented Oct 27, 2022 •

edited

Loading