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.

Sign up for GitHub

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

Jump to bottom

Expectations fix #35

Closed
wants to merge 13 commits into from
Closed

Expectations fix #35

wants to merge 13 commits into from

Conversation

wjhrdy
Copy link

@wjhrdy wjhrdy commented Oct 7, 2023 •
edited
Loading

This fixes #21 and one other bug when using the dbt_expectations package.

It adds the dbt_expectations test-suite which all runs without db errors but doesn't all pass weirdly.

The two macros that have overrides are.

CleanShot 2023-10-07 at 00 39 48

Also added Readme section to help future contributors.

hovaesco
hovaesco reviewed Oct 11, 2023
View reviewed changes
Copy link

@hovaesco hovaesco left a comment

Choose a reason for hiding this comment

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

(skimming)

docker-compose-starburst.yml Show resolved Hide resolved
README.md Outdated
@@ -91,3 +91,63 @@ or for a dry run to preview dropped models:
```bash
dbt run-operation trino__drop_old_relations --args "{dry_run: true}"
```

## Contributing
Copy link

Choose a reason for hiding this comment

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

I think we can move this section to a new file DEVELOPMENT.md

@hovaesco
Copy link

Please rebase since there are some conflicts.

@hovaesco
Copy link

Tests are not executed on the CI, please add them to Makefile.

@hovaesco
Copy link

something is failing in the CI https://github.com/starburstdata/dbt-trino-utils/actions/runs/6509366529/job/17732540173?pr=35#step:4:2696

@wjhrdy
Copy link
Author

wjhrdy commented Oct 16, 2023

Do you think This is because I am importing the source project and also I copied the tests from the source project into the integration tests folder?

hovaesco
hovaesco reviewed Oct 16, 2023
View reviewed changes
Makefile Show resolved Hide resolved
@hovaesco
Copy link

Do you think This is because I am importing the source project and also I copied the tests from the source project into the integration tests folder?

Yes it might be this, it's some conflict between two schema.yml

@damian3031
Copy link
Member

@wjhrdy Please pause your work on this PR. We are considering another approach, which involves adding Trino support directly to the dbt-expectations package rather than adding it to dbt-trino-utils. We believe this approach is better in terms of maintenance and is more user-friendly because users won't need to install an additional package or add extra configurations.

@wjhrdy
Copy link
Author

wjhrdy commented Oct 24, 2023

That works!

Just for your reference, it was recommended by the dbt-expectations people to add this to dbt-trino-utils

calogica/dbt-expectations#243 (comment)

@wjhrdy wjhrdy mentioned this pull request Oct 27, 2023
Closed
@wjhrdy wjhrdy closed this Dec 14, 2023
@damian3031
Copy link
Member

Trino is now supported directly in dbt_expectations.
relevant PR: calogica/dbt-expectations#294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hovaesco hovaesco hovaesco left review comments

@mdesmet mdesmet Awaiting requested review from mdesmet

@damian3031 damian3031 Awaiting requested review from damian3031

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request] Patch regexp_instr for Trino, so that its compatible in dbt-expectations
4 participants
@wjhrdy @hovaesco @damian3031 @aezomz