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.61k stars 1.59k forks source link

[Feature] Warn or error when intersection selection syntax includes a hanging `+ ` #10388

Open joellabes opened 2 months ago

joellabes commented 2 months ago

Is this a new bug in dbt-core?

Current Behavior

Slack thread

I accidentally included a space in my selector in Explorer: int_snowplow_unioned__core+,+ dbt_package_installs and got this first result:

image

When what I wanted was the intersection of the first model's children and the latter model's parents, int_snowplow_unioned__core+,+dbt_package_installs: image

I would have expected dbt to let me know that model+,+ is nonsensical, because there's nothing on the right hand plus to be upstream of. I reported this as an Explorer bug but they noted it matches Core's behaviour, so now I'm here

Expected Behavior

An error to be shown that I had created an invalid selection command

Steps To Reproduce

Have a chain of 3+ models that depend on one another Select them using the above syntax

Relevant log output

No response

Environment

- OS:
- Python:
- dbt:

Which database adapter are you using with dbt?

No response

Additional Context

No response

dbeatty10 commented 2 months ago

Good find @joellabes !

We've been reserving the bug label for when dbt is behaving in an obviously incorrect way and then using enhancement when dbt has room to improve somehow (like a more helpful error message). So I'm going to re-label this as an enhancement / feature request.

In the specific case, it is obviously non-sensical to do something like dbt list --select +. But in the general case, can you think if there are any practical rules/patterns for dbt to determine if XYZ below is sensical or not?

dbt list --select XYZ
joellabes commented 2 months ago

But in the general case, can you think if there are any practical rules/patterns for dbt to determine if XYZ below is sensical or not?

The main one that's on my mind here is that a graph operator should never be unaccompanied, so

dbt list --select +
dbt list --select 1+1 
dbt list --select @ 

should all fail.

Anything that is stringy could well be a model name, directory etc so I think we have to consider them sensical?

dbeatty10 commented 2 months ago

Creating some kind of regex for unaccompanied graph operators could definitely work. Then it could be expanded to cover other cases as-needed.

Do you have a take on the relative pros/cons of raising an error vs. raising a warning?

joellabes commented 2 months ago

Do you have a take on the relative pros/cons of raising an error vs. raising a warning?

Probably a warning! Very similar to NothingToDo or no nodes for selection

dbeatty10 commented 2 months ago

Acceptance criteria

  1. Create a regex that identifies unaccompanied graph operators (like described here)
  2. When selection matches the regex, then raise a warning similar to here