Skip to content

Minor: consolidate path_partition test into core_integration - #11831

Merged
lewiszlw merged 1 commit into
apache:mainfrom
alamb:alamb/consolidate
Aug 6, 2024
Merged

lewiszlw merged 1 commit into
apache:mainfrom
alamb:alamb/consolidate

Conversation

@alamb

@alamb alamb commented Aug 5, 2024

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

N/A

Rationale for this change

Each individual .rs file in the tests directory results in a new test binary which means:

  • 10s of MB of new binary (and thus slower)
  • It can't run in parallel with other tests (only tests within the same binary do)

What changes are included in this PR?

Consolidate the path_partition test into the core_integration test to make testing more efficient

Are these changes tested?

By CI. I also tested locally

You can see the tests run like this:

$ cargo test --test core_integration -- path_partition
   Compiling datafusion-physical-optimizer v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/physical-optimizer)
   Compiling datafusion v40.0.0 (/Users/andrewlamb/Software/datafusion/datafusion/core)
    Finished `test` profile [unoptimized + debuginfo] target(s) in 3.92s
     Running tests/core_integration.rs (target/debug/deps/core_integration-c1d878777c00ea56)

running 9 tests
test sql::path_partition::parquet_overlapping_columns ... ok
test sql::path_partition::parquet_statistics ... ok
test sql::path_partition::parquet_multiple_partitions ... ok
test sql::path_partition::parquet_multiple_nonstring_partitions ... ok
test sql::path_partition::csv_projection_on_partition ... ok
test sql::path_partition::csv_filter_with_file_nonstring_col ... ok
test sql::path_partition::csv_filter_with_file_col ... ok
test sql::path_partition::csv_grouping_by_partition ... ok
test sql::path_partition::parquet_distinct_partition_col ... ok

test result: ok. 9 passed; 0 failed; 0 ignored; 0 measured; 200 filtered out; finished in 0.07s

Are there any user-facing changes?

@alamb alamb changed the title Minor: move path_partition into core_integration Minor: consolidate path_partition test into core_integration Aug 5, 2024
@alamb
alamb force-pushed the alamb/consolidate branch from e4bc409 to bef4044 Compare August 5, 2024 20:31
@github-actions github-actions Bot added the core Core DataFusion crate label Aug 5, 2024

@lewiszlw lewiszlw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM.

@lewiszlw
lewiszlw merged commit 4a47dcb into apache:main Aug 6, 2024
@alamb
alamb deleted the alamb/consolidate branch August 6, 2024 12:43
@alamb

alamb commented Aug 6, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @lewiszlw

bvolpato pushed a commit to bvolpato/datafusion that referenced this pull request Oct 16, 2026
…26422)

## Which issue does this PR close?

This follows the same rationale as earlier PRs like
- apache#10588
- apache#11831
- apache#18226
- apache#26419

## Rationale for this change

Each `.rs` file directly in `datafusion/core/tests` becomes its own test
binary. In this case, the standalone `tpcds_planning` binary requires
**390 MB** (408,695,296 bytes, as reported by `du -h`) on my machine
(macOS, aarch64)

```shell
cargo test -p datafusion --test tpcds_planning --no-run
```

The binary was added in
- apache#4719

## What changes are included in this PR?

Move `datafusion/core/tests/tpcds_planning.rs` to
`datafusion/core/tests/sql/tpcds_planning.rs` and register it in
`sql/mod.rs`, so it is built as part of `core_integration`.

Size of `core_integration` (debug build):

| | bytes |
|---|---|
| before | 457,595,344 |
| after | 458,314,960 (+703 KB) |

So this saves creating a ~390 MB binary at a cost of ~703 KB in
`core_integration`.

## What is the testing strategy for this PR?
CI

## Are there any user-facing changes?

No, developer productivity / CI efficiency

Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants