Skip to content

Conversation

jsai28
Copy link
Contributor

@jsai28 jsai28 commented Mar 16, 2025

Which issue does this PR close?

Closes #15245.

Rationale for this change

What changes are included in this PR?

Migrated tests in datafusion/core/tests/dataframe to use insta.

Are these changes tested?

Are there any user-facing changes?

No.

@github-actions github-actions bot added the core Core DataFusion crate label Mar 16, 2025
@blaginin
Copy link
Contributor

blaginin commented Mar 17, 2025

Thank you! FYI to fix fmt, you can cargo fmt --all and commit changes. I also find it useful to use this:

datafusion/pre-commit.sh

Lines 22 to 23 in 87eec43

# Soft link it as git hook under top dir of apache arrow git repository:
# $ ln -s ../../pre-commit.sh .git/hooks/pre-commit

Comment on lines 78 to 81
async fn get_batches(expr: Expr, limit: usize) -> Result<Vec<RecordBatch>> {
let df = create_test_table().await?;
let df = df.select(vec![expr])?.limit(0, Some(limit))?;
df.collect().await
Copy link
Contributor

Choose a reason for hiding this comment

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

to fully replicate the original logic, you can also do get_batches(expr) and get_batches_with_limit(expr, limit) - that way you're reduce the magic number (10) across the codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes good point, I've updated the code with your suggestion.

Copy link
Contributor

@blaginin blaginin left a comment

Choose a reason for hiding this comment

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

🙏🙏🙏

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

THANK you @jsai28 and @blaginin for organizing this project and the review

@alamb alamb merged commit 5224194 into apache:main Mar 17, 2025
27 checks passed
@jsai28 jsai28 deleted the Migrate-dataframe-tests-to-insta branch March 17, 2025 18:45
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.

Migrate dataframe tests to insta

3 participants