Skip to content

Improve error message when the parsing of 'cluster_by' / 'partition_by' fields fails in the dbt loader#2548

Merged
izeigerman merged 1 commit intomainfrom
chore-dbt-loader-improve-error-msgs
May 1, 2024
Merged

Improve error message when the parsing of 'cluster_by' / 'partition_by' fields fails in the dbt loader#2548
izeigerman merged 1 commit intomainfrom
chore-dbt-loader-improve-error-msgs

Conversation

@izeigerman
Copy link
Contributor

Make the messages more specific about which field caused the failure.

@izeigerman izeigerman requested a review from a team May 1, 2024 17:19
Copy link
Contributor

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

LGTM - just a suggestion

Comment on lines +356 to +360
for c in self.cluster_by:
try:
clustered_by.append(d.parse_one(c, dialect=dialect).name)
except SqlglotError as e:
raise ConfigError(f"Failed to parse cluster_by field '{c}': {e}") from e
Copy link
Contributor

Choose a reason for hiding this comment

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

Small QOL - should we aggregate these errors so they're all reported at once after the loop instead of 1 by 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why? It seems like our general strategy has been failing fast

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that makes sense - I don't feel strongly about this, so feel free to ignore.

My rationale was that I generally prefer seeing all errors at once vs triggering another load of the project to find that another cluster key failed to parse. These would be picked up quickly, so in the literal sense it'd still be fast.

@izeigerman izeigerman merged commit 4db2a48 into main May 1, 2024
@izeigerman izeigerman deleted the chore-dbt-loader-improve-error-msgs branch May 1, 2024 22:47
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.

3 participants