You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "blaginin (via GitHub)" <gi...@apache.org> on 2025/04/21 13:27:36 UTC
[I] Migrate `core` tests to `insta` [datafusion]
blaginin opened a new issue, #15791:
URL: https://github.com/apache/datafusion/issues/15791
In https://github.com/apache/datafusion/issues/15178, we're switching hard-coded constants in tests to `insta`.
This issue targets updating **core tests** (`datafusion/core`).
Examples of tests that need to be switched to `insta`
- https://github.com/apache/datafusion/blob/91ad9fda2652f584d786a964ee205829e7a84580/datafusion/core/tests/sql/explain_analyze.rs#L177
- https://github.com/apache/datafusion/blob/91ad9fda2652f584d786a964ee205829e7a84580/datafusion/core/tests/sql/explain_analyze.rs#L211
- https://github.com/apache/datafusion/blob/91ad9fda2652f584d786a964ee205829e7a84580/datafusion/core/tests/sql/explain_analyze.rs#L72 has to be smart about that one as it only tests that plan contains some part - you may want to check [redactions](https://insta.rs/docs/redactions/) for that
- https://github.com/apache/datafusion/blob/91ad9fda2652f584d786a964ee205829e7a84580/datafusion/core/tests/expr_api/simplification.rs#L296
- https://github.com/apache/datafusion/blob/91ad9fda2652f584d786a964ee205829e7a84580/datafusion/core/tests/physical_optimizer/enforce_sorting.rs#L265
... and so on - check `assert_optimized!` `DataSourceExec:`, `assert_optimized_in_all_boundedness_situations`
Check [the original issue](https://github.com/apache/datafusion/issues/15178) for tips on how to do the switch and examples of completed PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-3439055215
Actually, update I see
-https://github.com/apache/datafusion/pull/18248
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-2934726143
> should I call assert_snapshot!() once for each line individually, or combine all the target lines into a single snapshot call?
I like the second option - check the whole logical plan with metrics in one big assert. There will be variable parts (e.g. `elapsed_compute=47.332µs`), but you can handle those using the [redaction](https://insta.rs/docs/redactions/#selectors).
> I modified it to return the matching lines from the plan
We switched to insta to be able to automatically update all snapshots when making a big formatting-related change. The issue with the "matching" approach is that automatic updates won't work - one would need to manually update the matching line to fix the tests.
--
> Only move the plan text checking from the macro and replace it with assert_snapshot!()
You can also keep `assert_snapshot` in place, just call `assert_snapshot` internally - [example](https://github.com/apache/datafusion/pull/15984/files#diff-c211e7f11e4de0cc04173e5e16ab9e6a97a72f1b0057690761f35eb154e27aa3R146)
--
Also, feel free to split this ticket into several PRs - I think that way you'll get feedback faster :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-3137676994
I just noticed we have the last chunk almost ready:
- https://github.com/apache/datafusion/pull/16978
❤️
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-3439051470
I didn't check all of the items above but I think we may have completed this ticket 🤞 -- perhaps we can close it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "Chen-Yuan-Lai (via GitHub)" <gi...@apache.org>.
Chen-Yuan-Lai commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-2940292401
@blaginin Thanks for your suggestion, I will try it soon.
> Also, feel free to split this ticket into several PRs - I think that way you'll get feedback faster :)
Sorry for the long delay. I'll push the first part of the changes mentioned above and create a PR for initial review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "blaginin (via GitHub)" <gi...@apache.org>.
blaginin commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-2931703475
Hey @Chen-Yuan-Lai, just checking if you have any questions about the issue or need any help :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed issue #15791: Migrate `core` tests to `insta`
URL: https://github.com/apache/datafusion/issues/15791
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "Chen-Yuan-Lai (via GitHub)" <gi...@apache.org>.
Chen-Yuan-Lai commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-2933783071
@blaginin Thank you for following up on this. I noticed some tests used a customized assertion macro, so I have two questions for this:
1. For `assert_metrics`: I'm unable to use inline `assert_snapshot!` within the macro, so I modified it to return the matching lines from the plan and use `assert_snapshot!()` (with filters to redact strings) in the test. I'm unsure about the snapshot strategy: should I call `assert_snapshot!()` once for each line individually, or combine all the target lines into a single snapshot call?
2. For `assert_optimized` in `physical_optimizer/enforce_sorting.rs`, the macro has logic for checking not only the plan text, but also the tree node integrity. In this case, should I:
a. Only move the plan text checking from the macro and replace it with `assert_snapshot!()`, or
b. Move and replace all the checks in the macro? This would require changing the `check_integrity()` function in `physical_optimizer/test_utils.rs`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Migrate `core` tests to `insta` [datafusion]
Posted by "Chen-Yuan-Lai (via GitHub)" <gi...@apache.org>.
Chen-Yuan-Lai commented on issue #15791:
URL: https://github.com/apache/datafusion/issues/15791#issuecomment-2850451010
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org