comphead commented on code in PR #6883: URL: https://github.com/apache/datafusion-comet/pull/6883#discussion_r4414768966
########## spark/src/main/scala/org/apache/comet/local/CometLocalRule.scala: ########## @@ -0,0 +1,152 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.comet.local + +import scala.jdk.CollectionConverters._ + +import org.apache.spark.sql.SparkSession +import org.apache.spark.sql.catalyst.expressions.{SortOrder, SubqueryExpression} +import org.apache.spark.sql.catalyst.plans.physical.RangePartitioning +import org.apache.spark.sql.comet.{CometNativeExec, CometRangeExec} +import org.apache.spark.sql.execution.{CollectLimitExec, QueryExecution, SparkPlan} +import org.apache.spark.sql.internal.SQLConf + +import com.google.protobuf.Descriptors.FieldDescriptor.JavaType +import com.google.protobuf.Message + +import org.apache.comet.CometConf +import org.apache.comet.serde.ExprOuterClass.Expr +import org.apache.comet.shims.ShimCometStreaming + +/** + * Admission of whole queries to local execution. A query is admitted only as a whole, from the + * plan Comet's ordinary conversion produces: one native block with no JVM input, whose operators + * then run for all partitions in one native graph. Everything else keeps ordinary execution. + */ +private[comet] object CometLocalRule { + + /** At most this many partitions run in one graph. */ + private val MaxPartitions = 1024 + + /** Expressions that call back into the JVM, which a local query does not do. */ + private val JvmCallbacks = Set(Expr.ExprStructCase.SUBQUERY, Expr.ExprStructCase.JVM_SCALAR_UDF) + + /** An already admitted local query, with or without its result boundary or limit wrapper. */ + def isLocal(plan: SparkPlan): Boolean = plan match { + case _: CometLocalExec | _: CometLocalResultExec => true + case limit: CollectLimitExec => isLocal(limit.child) + case _ => false + } + + /** + * `plan` as a local query if it is admitted, and otherwise as `convert`, Comet's ordinary + * conversion, produces it. A root `CollectLimitExec` (from `take`, `head` or `show`) is kept: + * it reads the result through `CometLocalResultExec.executeTake`. + */ + def apply( + session: SparkSession, + plan: SparkPlan, + convert: SparkPlan => SparkPlan): SparkPlan = { + if (!CometConf.COMET_EXEC_LOCAL_ENABLED + .get(SQLConf.get) || !environmentAdmits(session, plan)) { + return convert(plan) + } + val limit = plan match { + case limit: CollectLimitExec if limit.offset == 0 => Some(limit) Review Comment: Since b456e6d03, `CometLocalResultExec.executeTake` runs the block as ordinary Comet, so `take`, `head`, `show` and `limit(n).collect()`, which all reach this through `CollectLimitExec.executeCollect`, no longer use the local path, and the doc above still says they do. Admitting the limit now only changes `CollectLimitExec.doExecute`, which serves Spark Connect's collect, Arrow `toPandas`, `limit(n).toLocalIterator()` and `limit(n).rdd`. With one child partition, `doExecute` skips the per-partition `take` and the shuffle and returns the first `n` rows of the concatenated result, while Spark runs `take(n)` in every child partition. With ANSI on, I expect `spark.range(0, 6, 1, 3).selectExpr("100L DIV (4L - id) AS v").limit(1).toLocalIterator()` to fail with `DIVIDE_BY_ZERO` in Spark and to return `25` here. I have not run it. When the block is not admitted, line 83 also converts the whole plan a second time. Could a root `CollectLimitExec` be left to ordinary conversion? ########## native/local/src/lib.rs: ########## @@ -0,0 +1,357 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Query-scoped execution, independent of Spark tasks and JNI. +//! +//! A caller supplies a fresh, fully planned graph for each execution and receives one +//! result stream. The caller owns the Tokio runtime and must keep it running until +//! cleanup finishes. Spark admission and JNI live outside this crate. + +pub mod handle; + +use std::collections::VecDeque; +use std::panic::AssertUnwindSafe; +use std::pin::Pin; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex, MutexGuard}; +use std::task::{Context, Poll}; + +use datafusion::arrow::datatypes::SchemaRef; +use datafusion::arrow::record_batch::RecordBatch; +use datafusion::common::{DataFusionError, Result}; +use datafusion::execution::TaskContext; +use datafusion::physical_plan::stream::RecordBatchStreamAdapter; +use datafusion::physical_plan::{ + execute_stream, ExecutionPlan, ExecutionPlanProperties, RecordBatchStream, + SendableRecordBatchStream, +}; +use futures::future::poll_fn; +use futures::{FutureExt, Stream, StreamExt}; +use tokio::sync::Notify; +use tokio::task::JoinHandle; + +/// Batches each root partition may always hold ahead of delivery, when results are delivered in +/// partition order, whatever the read-ahead limit. +const PARTITION_BUFFER: usize = 2; + +/// One execution's graph and resources. Consumed when execution starts; not a plan cache. +pub struct LocalQuery { + plan: Arc<dyn ExecutionPlan>, + context: Arc<TaskContext>, + /// Delivers the partitions in order, with at most this many running at once and this many + /// bytes read ahead. + in_partition_order: Option<(usize, usize)>, Review Comment: The only non-test caller, `native/core/src/local.rs`, always sets `in_partition_order`, so the `execute_stream` branch never runs in Comet. Most of `tests/execution.rs` and several `tests/bridge.rs` tests build `LocalQuery::new(..).execute()` without it, so they mainly exercise DataFusion's `RepartitionExec` and `GlobalLimitExec` behind `execute_stream`. Could partition order be the only mode, with `max_running` and `read_ahead` passed to `LocalQuery::new`, and the coalescing mode and its tests land with the part that admits exchanges? ########## native/local/src/handle.rs: ########## @@ -0,0 +1,226 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +//! Bounded handoff to blocking callers, with cancellation independent of the reader lock. + +use std::panic::AssertUnwindSafe; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{mpsc, Arc, Mutex}; +use std::task::Poll; +use std::time::Duration; + +use datafusion::arrow::record_batch::RecordBatch; +use datafusion::common::{DataFusionError, Result}; +use futures::future::poll_fn; +use futures::{FutureExt, StreamExt}; +use tokio::runtime::Handle; +use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +use tokio::task::JoinHandle; + +use crate::{lock, LocalQuery, LocalQueryStream}; + +struct Delivery { + batch: Result<RecordBatch>, + _permit: OwnedSemaphorePermit, +} + +pub enum QueryPoll { + Batch(RecordBatch), + Pending, + Finished, +} + +/// Holds at most one queued batch, plus the batch owned by the external consumer. +/// Cancelling or dropping the handle drops the query's stream on the calling thread and +/// unread batches with it. The runtime must remain alive until DataFusion finishes aborting +/// the tasks its operators spawn. +pub struct QueryHandle { Review Comment: `QueryHandle` repeats the lifecycle of `BatchProducer` in `jni_api.rs`: a Tokio task polls the stream from an `Arc<Mutex<Option<_>>>`, a flag tells completion from cancellation, `catch_unwind` turns a panic into an error, and stop drops the stream on the calling thread. `Partition::spawn` in `lib.rs` is a third copy with a read-ahead budget, so each batch crosses two channels and two tasks before the JNI thread. Could the JNI pull poll `LocalQueryStream` directly with a bounded wait, as `executePlan` already does with `block_on` for JVM-fed plans? If the separate producer is needed so `close` can run during a pull, moving `BatchProducer` into `native/local` would leave one implementation. I have not prototyped either. -- 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]
