You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Shreyaskr1409 (via GitHub)" <gi...@apache.org> on 2025/03/23 11:43:18 UTC
[PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Shreyaskr1409 opened a new pull request, #15364:
URL: https://github.com/apache/datafusion/pull/15364
## Which issue does this PR close?
- Part of #15248.
## Rationale for this change
Completely migrate physical plan tests to `insta`
## What changes are included in this PR?
Migrating from `assert_batches_sorted_eq` and `assert_batches_eq` to `assert_snapshot` in physical-plan tests
## Are these changes tested?
Yes, they are
## Are there any user-facing changes?
No, they are not
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15364:
URL: https://github.com/apache/datafusion/pull/15364
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15364:
URL: https://github.com/apache/datafusion/pull/15364#issuecomment-2749131666
Passed on rerun. 🚀
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15364:
URL: https://github.com/apache/datafusion/pull/15364#discussion_r2009205442
##########
datafusion/physical-plan/src/repartition/mod.rs:
##########
@@ -1333,23 +1335,58 @@ mod tests {
let exec = RepartitionExec::try_new(Arc::new(input), partitioning).unwrap();
- let expected = vec![
- "+------------------+",
- "| my_awesome_field |",
- "+------------------+",
- "| foo |",
- "| bar |",
- "| frob |",
- "| baz |",
- "+------------------+",
- ];
-
- assert_batches_sorted_eq!(&expected, &expected_batches);
+ // The results are different from what is expected in snapshots thus i am not using
Review Comment:
I do't understand this comment. batches to string has the same result, so it seems that this is ok.
I think the difference as @blaginin pointed out on a previous PR is that the batches_to_sort_string sorts the otuput that appears in the test whereas `assert_batches_sorted_eq` sorts before comparison
So TLDR is I think you can just used `bathes_to_sort_string` and update the output
##########
datafusion/physical-plan/src/joins/sort_merge_join.rs:
##########
@@ -2280,22 +2280,22 @@ fn fetch_right_columns_from_batch_by_idxs(
.map(|column| take(column, &buffered_indices, None))
.collect::<Result<Vec<_>, ArrowError>>()
.map_err(Into::<DataFusionError>::into)?),
- // If the batch was spilled to disk, less likely
- (Some(spill_file), None) => {
- let mut buffered_cols: Vec<ArrayRef> =
- Vec::with_capacity(buffered_indices.len());
-
- let file = BufReader::new(File::open(spill_file.path())?);
- let reader = StreamReader::try_new(file, None)?;
-
- for batch in reader {
- batch?.columns().iter().for_each(|column| {
- buffered_cols.extend(take(column, &buffered_indices, None))
- });
- }
+ // If the batch was spilled to disk, less likely
Review Comment:
I don't understand why this code was changed (seems like the whitespace changes are not needed)
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15364:
URL: https://github.com/apache/datafusion/pull/15364#issuecomment-2749125261
> @alamb I updated everything. There are a few failing tests but they are failing due to time (lib) not getting compiled. This is unusual as tests passed the last time and I did not add any major changes which would make tests fail. Is there something more which I am supposed to do?
I retriggered the test job and hopefully they will pass this time (sometimes there is some sort of intermittent issue 🤷 )
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15364:
URL: https://github.com/apache/datafusion/pull/15364#issuecomment-2749132020
Thanks again @Shreyaskr1409 and @xudong963
--
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: [PR] Migrate physical plan tests to `insta` (Part-2) [datafusion]
Posted by "Shreyaskr1409 (via GitHub)" <gi...@apache.org>.
Shreyaskr1409 commented on PR #15364:
URL: https://github.com/apache/datafusion/pull/15364#issuecomment-2746414142
@alamb I updated everything. There are a few failing tests but they are failing due to time (lib) not getting compiled. This is unusual as tests passed the last time and I did not add any major changes which would make tests fail. Is there something more which I am supposed to do?
--
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