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.

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

Already on GitHub? Sign in to your account

Fix sqlglot dependency conflict with superset 4 and fix superset auth/reauth to use only the API #319

Open
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

joaopamaral
Copy link

There is a conflict with superset 4 dependencies and using only the API to authenticate it's better than using web scraping tool to avoid unexpected issues when the UI changes.

@joaopamaral
Copy link
Author

Hi @Vitor-Avila! Sorry, I've missed your comment.

I've bumped the sqlglot to use the same version of the latest superset and fixed some expression loops for the new version (now only tests/cli/superset/sync/dbt/metrics_test.py::test_replace_metric_syntax test is failing). As you said, the jinja template bug for dbt queries remains. I'll try to detect the block start {{ and block end }} and make the tokens between as a placeholder to allow it to run without changing the content. The {% %} is correctly identified as block start/end but the content between is defined as a variable and not placeholder (so it's failing to parse).

I've noticed that for this test we have a ParseError in the past (captured in the exception), but in the new sqlglot new version it parse both {{ }} as STRUCT(STRUCT()) and not raise this exception.

@joaopamaral
Copy link
Author

I've added the replace_jinja_tokens to make sqlglot identify {{/}} it as a block start/end.

def replace_jinja_tokens(tokens: List[Token]) -> List[Token]:
"""
Replaces Jinja-style `{{` and `}}` as block start/end.
Args:
tokens (List[Token]): List of tokens to process.
Returns:
List[Token]: List of tokens with Jinja blocks replaced.
"""

All tests are passing now.

setup.cfg Outdated Show resolved Hide resolved
@Vitor-Avila
Copy link
Contributor

Vitor-Avila commented Nov 29, 2024

Hey @joaopamaral,

Checking this PR, I can see the following changes:

  • sqlglot bump (and required fixes)
  • Changes to the Superset client
  • Changes to the Superset auth flow
  • Changes to the Makefile

I left some first pass comments, but I'm wondering if we should actually split this work into individual PRs? This would allow us to merge them individually as we make progress.

It seems that the CLA signature also didn't reflect properly FYI.

@joaopamaral
Copy link
Author

Hi @Vitor-Avila! Thanks for the review!
Sure we can split into more PRs!
I'll work on it during the weekend.

@Vitor-Avila
Copy link
Contributor

No rush @joaopamaral. And thank YOU for these contributions. We really appreciate it 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants