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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sqlparser: update code to conform to upstream sqlparser-rs changes #2510

Merged
merged 1 commit into from Apr 2, 2024
Merged

sqlparser: update code to conform to upstream sqlparser-rs changes #2510

merged 1 commit into from Apr 2, 2024

Conversation

harels
Copy link
Member

@harels harels commented Mar 13, 2024

Problem

馃憢 Thanks for opening a pull request! Please include a brief summary of the problem your change is trying to solve, or bug fix. If your change fixes a bug or you'd like to provide context on why you're making the change, please link the issue as follows:

Closes: #ISSUE-NUMBER

Solution

Please describe your change as it relates to the problem, or bug fix, as well as any dependencies. If your change requires a schema change, please describe the schema modification(s) and whether it's a backwards-incompatible or backwards-compatible change, then select one of the following:

Note: All schema changes require discussion. Please link the issue for context.

  • Your change modifies the core OpenLineage model
  • Your change modifies one or more OpenLineage facets

If you're contributing a new integration, please specify the scope of the integration and how/where it has been tested (e.g., Apache Spark integration supports S3 and GCS filesystem operations, tested with AWS EMR).

One-line summary:

Checklist

  • You've signed-off your work
  • Your pull request title follows our guidelines
  • Your changes are accompanied by tests (if relevant)
  • Your change contains a small diff and is self-contained
  • You've updated any relevant documentation (if relevant)
  • Your comment includes a one-liner for the changelog about the specific purpose of the change (if necessary)
  • You've versioned the core OpenLineage model or facets according to SchemaVer (if relevant)
  • You've added a header to source files (if relevant)

SPDX-License-Identifier: Apache-2.0
Copyright 2018-2023 contributors to the OpenLineage project

pawel-big-lebowski
pawel-big-lebowski requested changes Mar 26, 2024
View reviewed changes
Copy link
Contributor

@pawel-big-lebowski pawel-big-lebowski left a comment

Choose a reason for hiding this comment

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

SQL integration is covered with valuable tests that verify if an output from SQL Parser Is translated properly to TableLineage. Is there any particular reason for not doing so anymore?

integration/sql/impl/src/visitor.rs Show resolved Hide resolved
integration/sql/impl/src/visitor.rs Show resolved Hide resolved
integration/sql/impl/src/visitor.rs Show resolved Hide resolved
integration/sql/impl/src/visitor.rs Show resolved Hide resolved
@boring-cyborg boring-cyborg bot added area:ci CI area:client/java openlineage-java area:client/python openlineage-python area:documentation Improvements or additions to documentation area:tests Testing code labels Mar 29, 2024
@mobuchowski mobuchowski force-pushed the update-sqlparser-fork branch 2 times, most recently from 9b1791c to e1c69d5 Compare March 29, 2024 15:28
@mobuchowski mobuchowski changed the title test upstream changes from sql parser sqlparser: update code to conform to upstream sqlparser-rs changes Mar 29, 2024
@mobuchowski mobuchowski force-pushed the update-sqlparser-fork branch 2 times, most recently from 1a7f163 to dd7eb44 Compare April 1, 2024 17:07
@boring-cyborg boring-cyborg bot added the area:integration/common openlineage-integration-common label Apr 1, 2024
@boring-cyborg boring-cyborg bot added area:integration/airflow openlineage-airflow area:spec Specifications and standards for the project labels Apr 1, 2024
@mobuchowski mobuchowski force-pushed the update-sqlparser-fork branch 2 times, most recently from c0d11c2 to 70ed079 Compare April 1, 2024 23:45
pawel-big-lebowski
pawel-big-lebowski reviewed Apr 2, 2024
View reviewed changes
Copy link
Contributor

@pawel-big-lebowski pawel-big-lebowski left a comment

Choose a reason for hiding this comment

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

Thanks for writing the tests Maciej. Please double check release branch name.
Apart from that few cosmetic comments added.

integration/airflow/tests/integration/tests/airflow/dags/failed_sql_extraction.py
@@ -30,7 +30,7 @@


# Some sql-parser unsupported syntax
sql = "CREATE TYPE myrowtype AS (f1 int, f2 text, f3 numeric)"
sql = "DROP POLICY IF EXISTS name ON table_name"
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the advantage of replacing one test scenario with another?

integration/sql/impl/Cargo.toml
sqlparser = {git = "https://github.com/OpenLineage/sqlparser-rs/", branch = "fix_snowflake_stage_no_parens"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to rely on feature branch? If there is any particular reason to do that, pls specify PR that we need to merge in another repo. Otherwise it may be difficult to change this in future.

Copy link
Member

Choose a reason for hiding this comment

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

Because if we merge the sqlparser-rs changes first here: OpenLineage/sqlparser-rs#5 it would break the main: so the algorithm is

  • do the sqlparser change on a branch
  • make changes in the OpenLineage repo, pointing to the branch
  • merge sqlparser changes to our fork
  • move the changes back to the release branch on OpenLineage repo

integration/sql/impl/src/visitor.rs Show resolved Hide resolved
integration/sql/impl/tests/table_lineage/tests_select.rs Outdated Show resolved Hide resolved
@mobuchowski
sql: update after rebasing sqlparser-rs fork
3a90d21 
Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
@mobuchowski mobuchowski merged commit 9a8a3cf into main Apr 2, 2024
70 checks passed
@mobuchowski mobuchowski deleted the update-sqlparser-fork branch April 2, 2024 11:53
blacklight pushed a commit to blacklight/OpenLineage that referenced this pull request Apr 4, 2024
@harels @mobuchowski @blacklight
sql: update after rebasing sqlparser-rs fork (OpenLineage#2510)
54a2c4c 
Signed-off-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
Co-authored-by: Maciej Obuchowski <obuchowski.maciej@gmail.com>
Signed-off-by: Fabio Manganiello <fabio@manganiello.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mobuchowski mobuchowski mobuchowski left review comments

@pawel-big-lebowski pawel-big-lebowski pawel-big-lebowski approved these changes

Assignees
No one assigned
Labels
area:ci CI area:client/java openlineage-java area:client/python openlineage-python area:documentation Improvements or additions to documentation area:integration/airflow openlineage-airflow area:integration/common openlineage-integration-common area:integration/sql area:spec Specifications and standards for the project area:tests Testing code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@harels @pawel-big-lebowski @mobuchowski