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

feat: Metricflow and dbt Core #265

Merged
merged 3 commits into from
Mar 8, 2024
Merged

feat: Metricflow and dbt Core #265

merged 3 commits into from
Mar 8, 2024

Conversation

betodealmeida
Copy link
Member

@betodealmeida betodealmeida commented Mar 5, 2024

Add support for Metricflow when using dbt Core sync.

Ran:

$ preset-cli --workspaces=https://49437123.us1a.app.preset.io \
> superset sync dbt-core ~/Projects/github/jaffle-sl-template/target/manifest.json \
> --project=jaffle_shop --target=dev

See datasets and metrics in https://49437123.us1a.app.preset.io.

Will update tests.

Copy link

@Vitor-Avila
Copy link
Contributor

This is awesome!!!

@Vitor-Avila Vitor-Avila self-requested a review March 5, 2024 19:43
@Vitor-Avila
Copy link
Contributor

Followed below steps to test these changes:

  1. Created a new PSQL DB.
  2. Cloned this template repo: https://github.com/dbt-labs/jaffle-sl-template.
  3. Add the changes from this PR to avoid errors: Fix errors occur on mf validate-configs dbt-labs/jaffle-sl-template#62.
  4. Configured the DB connection to the new PSQL DB.
  5. Replaced DATETIME in the configs with TIMESTAMP (sice I'm using Postgres rather than Snowflake).
  6. Followed the steps outlined in the repo README to set up the dbt environment.

Comment on lines 364 to 382
except subprocess.CalledProcessError:
return None
Copy link
Contributor

@Vitor-Avila Vitor-Avila Mar 6, 2024

Choose a reason for hiding this comment

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

Now the CLI requires having metricflow installed to properly retrieve the SQL syntax for metrics. I wonder if we can identify the error returned in case it's not installed (maybe something like command not found and provide an error to the user instructing to install the package. We could raise it as an error to make sure a CI job would also show as failed so the user adds the specific metricflow package (according to their engine) to their configurations.

We could also add the package to the list of requirements.

Comment on lines +371 to +390
models = get_models_from_sql(sql, dialect, model_map)
if len(models) > 1:
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm working on this PR #266 and could potentially further improve it to also list at the end of the execution any failed metric (according to the limitations we currently have).

Copy link
Contributor

@Vitor-Avila Vitor-Avila left a comment

Choose a reason for hiding this comment

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

Thank you so much for working on this! I left some non-blocking comments.

@betodealmeida betodealmeida merged commit 5171892 into main Mar 8, 2024
4 checks passed
@betodealmeida betodealmeida deleted the sc-79017 branch March 8, 2024 01:32
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.

2 participants