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.

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

fix: better testing of lineage, and fix small bug with lineage tracking #4582

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

kgutwin
Copy link
Collaborator

@kgutwin kgutwin commented Jun 10, 2024

This improves the lineage tests (makes the snapshots more legible and has more comprehensive Python binding tests).

As part of this, a small bug for span assignments was discovered and fixed regarding assigning spans for nested pipelines.

This has the curious side effect of changing the "bad error message" for #3870 to be more specifically located to the aggregate portion of the failing query. I hope that this might help track down the underlying bug.

@kgutwin kgutwin requested a review from max-sixty June 10, 2024 15:07
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

This is indeed better, thanks!

I also wondered whether we should use to_json_pretty in our json methods, so the result had nicer line breaks. This solves some of that problem by using YAML.

Separate point but — it's a lot of lines; possible we should try and consolidate some of our integration tests, or run just a couple for anything apart from SQL — and low priority.

@kgutwin
Copy link
Collaborator Author

kgutwin commented Jun 10, 2024

I know it's a lot of snapshot lines, but I think that if lineage ever becomes a "real feature", we're going to want to test it consistently against the full span of queries. In fact, having this full test suite was already really useful in tracking down this little lineage bug, because it only occurred in rare cases; because it showed up in a couple of queries, it made it much more feasible to pin it down.

Thanks for the review!

@kgutwin kgutwin merged commit 164e82e into PRQL:main Jun 10, 2024
34 of 35 checks passed
@max-sixty
Copy link
Member

I know it's a lot of snapshot lines, but I think that if lineage ever becomes a "real feature", we're going to want to test it consistently against the full span of queries. In fact, having this full test suite was already really useful in tracking down this little lineage bug, because it only occurred in rare cases; because it showed up in a couple of queries, it made it much more feasible to pin it down.

OK great, that does make sense

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.

2 participants