dbt-labs / dbt-core

dbt enables data analysts and engineers to transform their data using the same practices that software engineers use to build applications.
https://getdbt.com
Apache License 2.0
9.72k stars 1.61k forks source link

[Feature] Improve Semantic Manifest parsing error message #9849

Open DevonFulcher opened 6 months ago

DevonFulcher commented 6 months ago

Is this your first time submitting a feature request?

Describe the feature

Improve this error message to provide more details as to what is wrong with the Semantic Manifest. This was brought up here (internal to dbt Labs).

Describe alternatives you've considered

No response

Who will this benefit?

Users who are trying to parse a semantic manifest.

Are you interested in contributing this feature?

No response

Anything else?

No response

dbeatty10 commented 6 months ago

Thanks for opening this issue @DevonFulcher !

It looks like the current error message is:

Semantic Manifest validation failed.

@Jstein77 do you have any recommendations for how we should proceed?

Jstein77 commented 5 months ago

@dbeatty10 great Q! It depends what the validation error was. @DevonFulcher we probably throw an error here in DSI, can we thread that error message through here?

Klimmy commented 4 months ago

Hey @DevonFulcher, @dbeatty10, @Jstein77,

What do you think about raising an error inside the validate() method and making it return None instead of bool? From what I checked, this behavior aligns with other validations in the project (example_1, example_2, example_3).

I've created a draft PR (#10128) with the implementation. If you think this approach is fine, I'll convert it to the normal PR.

Also, could you help me with these two points?

  1. Could you hint at how to reproduce this issue locally in dbt-core CLI? I've created a semantic model in yml file. "Forgot" to add primary type to entities (I believe it should trigger this error), and then executed dbt run. But everything worked fine in the main branch.
  2. I am hesitant about this point from the checklist. Could you help me understand if this change affects the interface? This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX
dbeatty10 commented 4 months ago

@Jstein77 can you answer question 1?

@Klimmy For question 2, this PR this PR has no interface changes, so I went ahead and updated that checklist item accordingly.

I've created a draft PR (https://github.com/dbt-labs/dbt-core/pull/10128) with the implementation. If you think this approach is fine, I'll convert it to the normal PR.

Go ahead and flip the PR from "draft" to "ready" whenever you want, and we'll queue it up for one of our engineers to review the approach.

Klimmy commented 4 months ago

Hey, no worries, I was able to test it using dbt --no-partial-parse run. Everything works as expected from my side. I will convert the PR to "ready". Thank you!