Page MenuHomePhabricator

Sqlfluff Rules for dbt
Closed, ResolvedPublic

Description

Sqlfluff is the linter recommended by dbt. We started copying the recommended rules from dbt, but teams are already using standards defined by them.

We need to define a Sqlfluff linter rules that works for all teams.

Task is done:

  • .sqlfluff is created in the dbt-jobs repository and all models in the main branch are following the rules.

Details

Related Changes in GitLab:
TitleReferenceAuthorSource BranchDest Branch
Linting Rulesrepos/data-engineering/dbt-jobs!18javiermontonfeature/new-linting-rulesmain
Customize query in GitLab

Related Objects

StatusSubtypeAssignedTask
OpenNone
OpenNone
ResolvedJMonton-WMF

Event Timeline

Edited description to add the data modeling guidelines. Since we are in the process of cleaning up the code I can try and implement some of the recommendations there.

For most of the rules, we can take the ones used in Airflow, but there are some rules that @Mayakp.wiki would like to change a bit.

@Mayakp.wiki has expressed concerns about having literals like true and false as lower case. The current Airflow sqlfluff configuration is forcing them to be lowercase, and sqlfluff fix will change them.

I think we could add the rule:

[sqlfluff:rules:capitalisation.literals]
capitalisation_policy = consistent

Which forces a file to be consistent, either using lower case or upper case.

Another issue we are seeing in Maya's code is related to the rule references.from or RF01. Although it is a good rule, it has false positives when working with struct fields. The documentation says it's disabled by default for dialect = sparksql but apparently, it's enabled. I think we could disable it.

Maya has also proposed to exclude month from the list of keywords, to avoid breaking RF04 when that keywords is used as alias, as they use it often.

cc: @amastilovic

Thanks for working through these details, @JMonton-WMF and @Mayakp.wiki !

  • Boolean literals question, I'm perfectly fine with having the capitalisation_policy set to consistent.
  • The RF01 rule about references was an issue back when we worked on the linting rules for Airflow/HQL, too. Not sure why it's not auto-disabled for dialect = sparksql so let's explicitly disable it in exclude_rules.
  • As far as RF04 goes - yes, and we will most likely have to keep a bigger list of excluded keywords since our SQL/HQL uses a lot of keywords as column/field names.

I ran sqlfluff on the cluster today using the steps @JMonton-WMF provided 2 days ago and was successfully able to pass all the linter rules in the current version of the model.
I also updated the sqlfluff ignore file and submitted a merge request for the change