Skip to content
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.

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

Update physical_plan tests to not use SessionContext #7243

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Aug 9, 2023

Which issue does this PR close?

Part of #1754

Rationale for this change

As part of preparing (finally) to move the physical_plan into its own crate so datafusion builds faster and is more modular, I need to cut all physical_plan --> core dependencies. SessionContext is in core

What changes are included in this PR?

  1. Rewrite most tests in physical_plan to use TaskContext (which is what they mostly need anyways)
  2. Move integration tests (that are testing running SQL) into either sqllogictest or core_integration

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Aug 9, 2023
@@ -1449,8 +1447,7 @@ mod tests {
DataType::Int64,
))];

let session_ctx = SessionContext::new();
let task_ctx = session_ctx.task_ctx();
let task_ctx = Arc::new(TaskContext::default());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pattern is 95% of this PR: create the TaskContext directly rather than via a SessionContext

use crate::test::create_vec_batches;
use arrow::datatypes::{DataType, Field, Schema};

#[tokio::test]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are tests that a config option makes it through to planning SQL, so I moved them to options.slt

@@ -1811,173 +1799,14 @@ mod tests {
Ok(())
}

#[tokio::test]
async fn join_change_in_planner() -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these are SQL level tests that need to entire SessionContext so I moved it to the core_integration test

@@ -1231,7 +1230,6 @@ mod tests {
DisplayAs, ExecutionPlan, Partitioning, RecordBatchStream,
SendableRecordBatchStream, Statistics,
};
use crate::prelude::SessionContext;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The point of this PR is to remove these dependencies

@yjshen yjshen merged commit a08ce40 into apache:main Aug 9, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants