Skip to content

Migrate core test to insta, part1 #16324

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

Merged
blaginin merged 7 commits into from
Jun 17, 2025
Merged

Migrate core test to insta, part1 #16324

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a88b86d
feat: migrate core test into insta
Jun 4, 2025
0f719c2
feat: enhance tests with snapshot assertions and remove unused macros
Jun 4, 2025
c8dd007
feat: rewrite snapshot test in explain_analyze_baseline_metrics
Jun 5, 2025
4a6ac4a
fix: update snapshot test to normalize file paths in explain_analyze_…
Jun 10, 2025
e95ed80
refactor: simplify snapshot assertions by removing format calls in op…
Jun 13, 2025
c981e70
feat: revert the original usage of assert_metrics macro in explain_a…
Jun 14, 2025
ef81186
Merge branch 'main' into migrate_core_test_to_insta
blaginin Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
feat: rewrite snapshot test in explain_analyze_baseline_metrics
  • Loading branch information
Cheng-Yuan-Lai committed Jun 14, 2025
commit c8dd007a8ba2e6fc9b082e64f7dcb8ab27ecc277
1 change: 1 addition & 0 deletions datafusion/core/tests/sql/aggregates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use super::*;
use datafusion::common::test_util::batches_to_string;
use datafusion_catalog::MemTable;
use datafusion_common::ScalarValue;
use insta::assert_snapshot;

#[tokio::test]
async fn csv_query_array_agg_distinct() -> Result<()> {
Expand Down
46 changes: 23 additions & 23 deletions datafusion/core/tests/sql/explain_analyze.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,38 +57,38 @@ async fn explain_analyze_baseline_metrics() {

println!("Query Output:\n\n{formatted}");

let re = Regex::new(r"\|[^|]*\|([^|]?)\s\|").unwrap();
let re = Regex::new(r"\|[^|]*\|([^|]*)\|").unwrap();
let actual = formatted
.lines()
.map(|line| re.replace_all(line, "$1").to_string())
.map(|line| re.replace_all(line, "$1").trim_end().to_string())
.filter(|line| !line.is_empty() && !line.starts_with('+'))
.collect::<Vec<_>>()
.join("\n");
insta::with_settings!({filters => vec![
(r"\d+\.?\d*[µmn]?s", "[TIME]"),
Copy link
Collaborator

@blaginin blaginin Jun 9, 2025

Choose a reason for hiding this comment

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

[µmn] - I think this can be somewhat flaky it the test takes more time - maybe match on []?

Copy link
Contributor Author

@Chen-Yuan-Lai Chen-Yuan-Lai Jun 11, 2025

Choose a reason for hiding this comment

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

Agree, current pattern can't match the longer time (ex. min, h, d). However, a broader pattern might match irrelevant strings. For example, match on [ ]: metrics=[output_rows=1, elapsed_compute=110.947µs] -> metrics=[TIME]

I'm currently considering to add more time units in the regex pattern:

r"\d+\.?\d*(?:µs|ms|ns|s|min|h)\b"

Or, do you have any suggestions for a more precise way to match time-related strings? many thanks.

Copy link
Contributor

@alamb alamb Jun 11, 2025

Choose a reason for hiding this comment

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

Maybe just for this teset case we could avoid using insta and use the previous approach of substring matches

While that approach is also non ideal, it has seemed to work this far

Copy link
Collaborator

@blaginin blaginin Jun 11, 2025

Choose a reason for hiding this comment

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

Happy with keeping as is, but on this

might match irrelevant strings

you can probably use lookahead https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Lookahead_assertion

Copy link
Contributor Author

@Chen-Yuan-Lai Chen-Yuan-Lai Jun 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

you can probably use lookahead https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Lookahead_assertion

Unfortunately, Rust's regex crate does not support lookahead or lookbehind assertions 😢 ( ref )

]}, {
insta::assert_snapshot!(actual,@r#"
plan
CoalescePartitionsExec: fetch=3, metrics=[output_rows=3, elapsed_compute=[TIME]]
UnionExec, metrics=[output_rows=3, elapsed_compute=[TIME]]
ProjectionExec: expr=[count(Int64(1))@0 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
AggregateExec: mode=Final, gby=[], aggr=[count(Int64(1))], metrics=[output_rows=1, elapsed_compute=[TIME]]
CoalescePartitionsExec, metrics=[output_rows=3, elapsed_compute=[TIME]]
AggregateExec: mode=Partial, gby=[], aggr=[count(Int64(1))], metrics=[output_rows=3, elapsed_compute=[TIME]]
ProjectionExec: expr=[], metrics=[output_rows=5, elapsed_compute=[TIME]]
AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[], metrics=[output_rows=5, elapsed_compute=[TIME], spill_count=0, spilled_bytes=0.0 B, spilled_rows=0, peak_mem_used=50592]
CoalesceBatchesExec: target_batch_size=4096, metrics=[output_rows=5, elapsed_compute=[TIME]]
RepartitionExec: partitioning=Hash([c1@0], 3), input_partitions=3, metrics=[fetch_time=[TIME], repartition_time=[TIME], send_time=[TIME]]
AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[], metrics=[output_rows=5, elapsed_compute=[TIME], spill_count=0, spilled_bytes=0.0 B, spilled_rows=0, skipped_aggregation_rows=0, peak_mem_used=52216]
CoalesceBatchesExec: target_batch_size=4096, metrics=[output_rows=99, elapsed_compute=[TIME]]
FilterExec: c13@1 != C2GT5KVyOPZpgKVl110TyZO0NcJ434, projection=[c1@0], metrics=[output_rows=99, elapsed_compute=[TIME]]
RepartitionExec: partitioning=RoundRobinBatch(3), input_partitions=1, metrics=[fetch_time=[TIME], repartition_time=[TIME], send_time=[TIME]]
DataSourceExec: file_groups={1 group: [[home/ian/open_source/datafusion/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c13], file_type=csv, has_header=true, metrics=[output_rows=100, elapsed_compute=[TIME], file_open_errors=0, file_scan_errors=0, time_elapsed_opening=[TIME], time_elapsed_processing=[TIME], time_elapsed_scanning_total=[TIME], time_elapsed_scanning_until_data=[TIME]]
ProjectionExec: expr=[1 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
PlaceholderRowExec, metrics=[]
ProjectionExec: expr=[lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@1 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
BoundedWindowAggExec: wdw=[lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Ok(Field { name: "lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)), is_causal: false }], mode=[Sorted], metrics=[output_rows=1, elapsed_compute=[TIME]]
ProjectionExec: expr=[1 as c1], metrics=[output_rows=1, elapsed_compute=[TIME]]
plan
CoalescePartitionsExec: fetch=3, metrics=[output_rows=3, elapsed_compute=[TIME]]
UnionExec, metrics=[output_rows=3, elapsed_compute=[TIME]]
ProjectionExec: expr=[count(Int64(1))@0 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
AggregateExec: mode=Final, gby=[], aggr=[count(Int64(1))], metrics=[output_rows=1, elapsed_compute=[TIME]]
CoalescePartitionsExec, metrics=[output_rows=3, elapsed_compute=[TIME]]
AggregateExec: mode=Partial, gby=[], aggr=[count(Int64(1))], metrics=[output_rows=3, elapsed_compute=[TIME]]
ProjectionExec: expr=[], metrics=[output_rows=5, elapsed_compute=[TIME]]
AggregateExec: mode=FinalPartitioned, gby=[c1@0 as c1], aggr=[], metrics=[output_rows=5, elapsed_compute=[TIME], spill_count=0, spilled_bytes=0.0 B, spilled_rows=0, peak_mem_used=50592]
CoalesceBatchesExec: target_batch_size=4096, metrics=[output_rows=5, elapsed_compute=[TIME]]
RepartitionExec: partitioning=Hash([c1@0], 3), input_partitions=3, metrics=[fetch_time=[TIME], repartition_time=[TIME], send_time=[TIME]]
AggregateExec: mode=Partial, gby=[c1@0 as c1], aggr=[], metrics=[output_rows=5, elapsed_compute=[TIME], spill_count=0, spilled_bytes=0.0 B, spilled_rows=0, skipped_aggregation_rows=0, peak_mem_used=52216]
CoalesceBatchesExec: target_batch_size=4096, metrics=[output_rows=99, elapsed_compute=[TIME]]
FilterExec: c13@1 != C2GT5KVyOPZpgKVl110TyZO0NcJ434, projection=[c1@0], metrics=[output_rows=99, elapsed_compute=[TIME]]
RepartitionExec: partitioning=RoundRobinBatch(3), input_partitions=1, metrics=[fetch_time=[TIME], repartition_time=[TIME], send_time=[TIME]]
DataSourceExec: file_groups={1 group: [[home/ian/open_source/datafusion/testing/data/csv/aggregate_test_100.csv]]}, projection=[c1, c13], file_type=csv, has_header=true, metrics=[output_rows=100, elapsed_compute=[TIME], file_open_errors=0, file_scan_errors=0, time_elapsed_opening=[TIME], time_elapsed_processing=[TIME], time_elapsed_scanning_total=[TIME], time_elapsed_scanning_until_data=[TIME]]
ProjectionExec: expr=[1 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
PlaceholderRowExec, metrics=[]
ProjectionExec: expr=[lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@1 as cnt], metrics=[output_rows=1, elapsed_compute=[TIME]]
BoundedWindowAggExec: wdw=[lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Ok(Field { name: "lead(b.c1,Int64(1)) ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING", data_type: Int64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)), is_causal: false }], mode=[Sorted], metrics=[output_rows=1, elapsed_compute=[TIME]]
ProjectionExec: expr=[1 as c1], metrics=[output_rows=1, elapsed_compute=[TIME]]
PlaceholderRowExec, metrics=[]
"#);
});
Expand Down
1 change: 1 addition & 0 deletions datafusion/core/tests/sql/select.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

use super::*;
use datafusion_common::ScalarValue;
use insta::assert_snapshot;

#[tokio::test]
async fn test_list_query_parameters() -> Result<()> {
Expand Down