andygrove commented on code in PR #2819:
URL:
https://github.com/apache/datafusion-ballista/pull/2819#discussion_r4507119941
##########
ballista/scheduler/src/state/aqe/test/physical_submission.rs:
##########
@@ -22,43 +22,23 @@
//! must keep those shuffles as stage boundaries and must not make decisions
//! that assume it knows what an extension operator does with its input.
-use crate::state::aqe::adaptive_rules::{AdaptiveRule, AdaptiveRules};
-use crate::state::aqe::execution_plan::ExchangeExec;
use crate::state::aqe::planner::AdaptivePlanner;
-use crate::state::aqe::test::coalesce_rule::partitions_with_byte_sizes;
-use crate::state::aqe::test::mock_batch;
+use crate::state::aqe::test::{
+ Embedder, Seen, coalesce_context, mock_batch, partitions_with_byte_sizes,
+};
+use crate::test_utils::PassThroughExec;
use ballista_core::assert_plan;
-use ballista_core::extension::SessionConfigExt;
-use datafusion::common::tree_node::{TreeNode, TreeNodeRecursion};
-use datafusion::config::ConfigOptions;
-use datafusion::execution::{SessionStateBuilder, TaskContext};
-use datafusion::physical_expr::PhysicalExpr;
use datafusion::physical_expr::expressions::Column;
-use datafusion::physical_optimizer::PhysicalOptimizerRule;
use datafusion::physical_plan::repartition::RepartitionExec;
-use datafusion::physical_plan::{
- DisplayAs, DisplayFormatType, ExecutionPlan, Partitioning, PlanProperties,
- SendableRecordBatchStream,
-};
-use datafusion::prelude::{SessionConfig, SessionContext};
-use std::fmt::Formatter;
+use datafusion::physical_plan::{ExecutionPlan, Partitioning};
+use datafusion::prelude::SessionContext;
use std::sync::Arc;
-use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
+use std::sync::atomic::Ordering;
-/// A session with Ballista's config extension installed, partition
-/// coalescing switched on or off, and a small coalesce target so the
-/// bin-pack is hand-traceable.
+/// A coalesce-tuned session with partition coalescing switched on or off and
+/// the table `t` registered.
fn physical_context(coalesce_enabled: bool) -> SessionContext {
- let config = SessionConfig::new_with_ballista()
- .with_target_partitions(8)
- .with_round_robin_repartition(false)
- .with_ballista_coalesce_enabled(coalesce_enabled)
- .with_ballista_coalesce_target_partition_bytes(200);
- let state = SessionStateBuilder::new()
- .with_config(config)
- .with_default_features()
- .build();
- let ctx = SessionContext::new_with_state(state);
+ let ctx = coalesce_context(8, coalesce_enabled);
Review Comment:
`physical_context` now goes through `coalesce_context`, which also sets
`prefer_hash_join=false` and zeroes the hash join thresholds. That has no
effect today because these plans have no joins. But a join test added here
later would quietly plan a sort-merge join. Maybe add a short note on
`physical_context`, or keep the join knobs out of the shared helper?
##########
ballista/scheduler/src/test_utils.rs:
##########
@@ -129,7 +104,12 @@ impl TableProvider for PanickingTableProvider {
_filters: &[Expr],
_limit: Option<usize>,
) -> datafusion::common::Result<Arc<dyn ExecutionPlan>> {
- panic!("PanickingTableProvider just panics!")
+ match self {
+ Self::Error => Err(DataFusionError::Plan(
+ "ExplodingTableProvider just throws an error!".to_owned(),
+ )),
+ Self::Panic => panic!("PanickingTableProvider just panics!"),
Review Comment:
Nice job folding the two providers into one enum. The `Panic` arm still
panics with `"PanickingTableProvider just panics!"`, and
`scheduler_server/mod.rs` asserts on that string. Would it be worth renaming it
to `"ExplodingTableProvider just panics!"` so the message matches the type?
Totally optional.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
