Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/workflows/pr_build_linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,7 @@ jobs:
org.apache.spark.sql.CometCollationSuite
org.apache.comet.CometFuzzAggregateSuite
org.apache.spark.sql.comet.execution.arrow.CometArrowStreamSuite
org.apache.spark.sql.comet.execution.arrow.CachedBatchRowIteratorSuite
org.apache.spark.sql.CometSparkInternalFunctionsSuite
- name: "expressions"
value: |
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/pr_build_macos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ jobs:
org.apache.spark.sql.CometCollationSuite
org.apache.comet.CometFuzzAggregateSuite
org.apache.spark.sql.comet.execution.arrow.CometArrowStreamSuite
org.apache.spark.sql.comet.execution.arrow.CachedBatchRowIteratorSuite
org.apache.spark.sql.CometSparkInternalFunctionsSuite
- name: "expressions"
value: |
Expand Down
20 changes: 12 additions & 8 deletions spark/src/main/scala/org/apache/comet/CometConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -262,16 +262,20 @@ object CometConf extends ShimCometConf {
conf("spark.comet.exec.inMemoryCache.enabled")
.category(CATEGORY_EXEC)
.doc(
"Whether to enable Comet native execution for in-memory cached tables. Its value at " +
"startup also decides whether CometDriverPlugin installs Comet's cache serializer, " +
"which stores cached data in Arrow format. Because spark.sql.cache.serializer is a " +
"Whether to enable Comet native scans and fused Spark reads of in-memory cached tables. " +
"Requires spark.comet.enabled=true. At startup, this setting also decides whether " +
"CometDriverPlugin installs Comet's cache serializer, which stores cached data in " +
"Arrow format. Because spark.sql.cache.serializer is a " +
"static config, the cached format is fixed for the application, and disabling this " +
"at runtime only sends cached scans back to Spark's execution path. Relations whose " +
"schema Comet's Arrow writer does not support are always cached in Spark's default " +
"or spark.comet.enabled at runtime sends cached scans back to Spark's execution path " +
"without the fused reader. Relations whose schema Comet's Arrow writer does not " +
"support are always cached in Spark's default " +
"format. Each cached column is stored as its own compressed Arrow IPC stream, so a " +
"scan decodes only the columns it projected. Reads that feed Spark operators rather " +
"than Comet ones still pay a row conversion the default format avoids, and can be " +
"slower than Spark's cache. With spark.kryo.registrationRequired=true, also set " +
"scan decodes only the columns it projected. Eligible Spark whole-stage codegen " +
"consumers read cached vectors directly when vectorized cache reading is enabled; " +
"other Spark row consumers use a reusable row buffer. Decoding costs can still make " +
"wide numeric reads slower than Spark's default cache. With " +
"spark.kryo.registrationRequired=true, also set " +
"spark.kryo.registrator=org.apache.comet.CometKryoRegistrator before creating the " +
"SparkContext, otherwise caching fails as soon as a block is serialized, including " +
"the disk half of the default MEMORY_AND_DISK storage level.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import org.apache.spark.sql.internal.SQLConf

import org.apache.comet.CometConf._
import org.apache.comet.iceberg.IcebergWriteStrategy
import org.apache.comet.rules.{CometExecRule, CometPlanAdaptiveDynamicPruningFilters, CometReuseSubquery, CometScanRule, CometSpark34AqeDppFallbackRule, EliminateRedundantTransitions, RevertNativeForTransitionHeavyStages}
import org.apache.comet.rules.{CometCacheColumnarRule, CometExecRule, CometPlanAdaptiveDynamicPruningFilters, CometReuseSubquery, CometScanRule, CometSpark34AqeDppFallbackRule, EliminateRedundantTransitions, RevertNativeForTransitionHeavyStages}
import org.apache.comet.shims.ShimCometSparkSessionExtensions

/**
Expand All @@ -54,7 +54,7 @@ import org.apache.comet.shims.ShimCometSparkSessionExtensions
* CometSubqueryBroadcastExec for exchange reuse with Comet broadcasts
* b. insertTransitions: ColumnarToRow/RowToColumnar added
* c. postColumnarTransitions: RevertNativeForTransitionHeavyStages,
* EliminateRedundantTransitions
* EliminateRedundantTransitions, CometCacheColumnarRule
* 5. ReuseExchangeAndSubquery -- Spark deduplicates subqueries (sees Comet nodes)
* }}}
*
Expand All @@ -78,7 +78,7 @@ import org.apache.comet.shims.ShimCometSparkSessionExtensions
* a. preColumnarTransitions: CometScanRule, CometExecRule (no-ops, already converted)
* b. insertTransitions
* c. postColumnarTransitions: RevertNativeForTransitionHeavyStages,
* EliminateRedundantTransitions
* EliminateRedundantTransitions, CometCacheColumnarRule
* }}}
*
* On Spark 3.4, injectQueryStageOptimizerRule is unavailable. CometExecRule does not wrap SABs,
Expand Down Expand Up @@ -113,7 +113,10 @@ class CometSparkSessionExtensions

override def postColumnarTransitions: Rule[SparkPlan] = {
val rules =
Seq(RevertNativeForTransitionHeavyStages(session), EliminateRedundantTransitions(session))
Seq(
RevertNativeForTransitionHeavyStages(session),
EliminateRedundantTransitions(session),
CometCacheColumnarRule)
plan => rules.foldLeft(plan) { case (p, rule) => rule(p) }
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* 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.rules

import org.apache.spark.sql.catalyst.expressions.LeafExpression
import org.apache.spark.sql.catalyst.expressions.codegen.CodegenFallback
import org.apache.spark.sql.catalyst.rules.Rule
import org.apache.spark.sql.comet.execution.arrow.ArrowCachedBatchSerializer
import org.apache.spark.sql.execution.{CodegenSupport, ColumnarToRowExec, ColumnarToRowTransition, SparkPlan, WholeStageCodegenExec}
import org.apache.spark.sql.execution.adaptive.QueryStageExec
import org.apache.spark.sql.execution.columnar.InMemoryTableScanExec

import org.apache.comet.CometConf.COMET_EXEC_IN_MEMORY_CACHE_ENABLED
import org.apache.comet.CometSparkSessionExtensions.isCometLoaded

/**
* Lets Spark's generated consumers read cached Arrow vectors without an intermediate UnsafeRow.
*
* Data flows upward. Spark's InputAdapter/whole-stage wrappers and an optional AQE cache stage
* are omitted:
* {{{
* Before After
* +------------------------+ +------------------------+
* | Spark codegen consumer | | Spark codegen consumer |
* +------------------------+ +------------------------+
* ^ ^
* | UnsafeRow | column values
* +------------------------+ +------------------------+
* | InMemoryTableScanExec | | ColumnarToRowExec |
* | row iterator | | fused with consumer |
* +------------------------+ +------------------------+
* ^
* | ColumnarBatch
* +------------------------+
* | InMemoryTableScanExec |
* | Arrow vectors |
* +------------------------+
* }}}
*/
object CometCacheColumnarRule extends Rule[SparkPlan] {
override def apply(plan: SparkPlan): SparkPlan = {
if (!isCometLoaded(conf) || !COMET_EXEC_IN_MEMORY_CACHE_ENABLED.get(conf)) return plan
if (!conf.wholeStageEnabled) return plan

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.

Thanks for adding the enable-switch guards and the runtime toggle test, that addresses my earlier comment. One more gate question. CollapseCodegenStages.apply only inserts whole-stage codegen when spark.sql.codegen.factoryMode is not NO_CODEGEN as well as wholeStageEnabled. Should this rule check the same thing? Otherwise under NO_CODEGEN with whole-stage on we insert a ColumnarToRowExec that never fuses and runs its plain doExecute. The existing tests always pair NO_CODEGEN with whole-stage off, so it might be worth adding that combination once the gate matches.


plan.transformUp {
case parent: CodegenSupport
if parent.supportCodegen && !parent.supportsColumnar &&
!parent.isInstanceOf[ColumnarToRowTransition] &&
!WholeStageCodegenExec.isTooManyFields(conf, parent.schema) &&
!parent.children.exists(p => WholeStageCodegenExec.isTooManyFields(conf, p.schema)) &&
!parent.expressions.exists(_.exists {
case _: LeafExpression => false
case _: CodegenFallback => true
case _ => false
}) =>
// Match the consuming edge rather than every scan: an existing columnar consumer (or a
// cache stage being materialized by AQE) must keep receiving batches. Spark inserts an
// InputAdapter around the scan later, while this transition fuses with the row consumer.
parent.withNewChildren(parent.children.map {
case child if isColumnarCometCache(child) => ColumnarToRowExec(child)
case child => child
})
}
}

private def isColumnarCometCache(plan: SparkPlan): Boolean = {
plan.supportsColumnar && (plan match {
case scan: InMemoryTableScanExec =>
// The serializer delegates unsupported schemas to Spark, whose cache keeps its own reader.
scan.relation.cacheBuilder.serializer.isInstanceOf[ArrowCachedBatchSerializer] &&
ArrowCachedBatchSerializer.supportsSchema(scan.relation.output)
case stage: QueryStageExec => isColumnarCometCache(stage.plan)
case _ => false
})
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,12 @@ package org.apache.spark.sql.comet.execution.arrow

import java.lang.{Boolean => JBoolean, Byte => JByte, Double => JDouble, Float => JFloat, Integer => JInteger, Long => JLong, Short => JShort}

import scala.collection.JavaConverters._
import scala.util.control.NonFatal

import org.apache.spark.TaskContext
import org.apache.spark.rdd.RDD
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions.{Attribute, Expression, GenericInternalRow, IsNotNull, IsNull, UnsafeProjection}
import org.apache.spark.sql.catalyst.expressions.{Attribute, Expression, GenericInternalRow, IsNotNull, IsNull}
import org.apache.spark.sql.catalyst.util.TypeUtils
import org.apache.spark.sql.columnar.{CachedBatch, SimpleMetricsCachedBatch, SimpleMetricsCachedBatchSerializer}
import org.apache.spark.sql.comet.util.Utils
Expand Down Expand Up @@ -646,11 +645,7 @@ class ArrowCachedBatchSerializer extends SimpleMetricsCachedBatchSerializer {

convertCachedBatchToColumnarBatch(input, cacheAttributes, selectedAttributes, conf)
.mapPartitions { batches =>
val toUnsafe = UnsafeProjection.create(selectedAttributes, selectedAttributes)

batches.flatMap { batch =>
batch.rowIterator().asScala.map(row => toUnsafe(row).copy())
}
new CachedBatchRowIterator(selectedAttributes).createObject(batches)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
/*
* 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.spark.sql.comet.execution.arrow

import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.expressions.{Attribute, BoundReference, CodeGeneratorWithInterpretedFallback, InterpretedUnsafeProjection}
import org.apache.spark.sql.catalyst.expressions.codegen._
import org.apache.spark.sql.catalyst.expressions.codegen.Block._
import org.apache.spark.sql.vectorized.{ColumnarBatch, ColumnVector}

/**
* Reads vectors directly into Spark's reusable UnsafeRow buffer. The input iterator owns the
* batches and releases them on advancement or task completion. As with Spark's cache reader,
* callers must copy rows they retain across next(), but the returned row owns its variable-width
* values and remains valid when hasNext() releases the batch that supplied them.
*/
private[arrow] class CachedBatchRowIterator(attributes: Seq[Attribute])
extends CodeGeneratorWithInterpretedFallback[Iterator[ColumnarBatch], Iterator[InternalRow]] {

private def fields: Seq[BoundReference] = attributes.zipWithIndex.map { case (attr, i) =>
BoundReference(i, attr.dataType, attr.nullable)
}

override protected def createCodeGeneratedObject(
batches: Iterator[ColumnarBatch]): Iterator[InternalRow] = {
val ctx = new CodegenContext
val columns = attributes.indices.map { i =>
ctx.addMutableState(classOf[ColumnVector].getName, s"column$i")
}
ctx.currentVars = attributes.zip(columns).map { case (attr, column) =>
val value = JavaCode.variable(ctx.freshName("value"), attr.dataType)
val getter = CodeGenerator.getValueFromVector(column, attr.dataType, "rowId")
val javaType = CodeGenerator.javaType(attr.dataType)
if (attr.nullable) {
val isNull = JavaCode.isNullVariable(ctx.freshName("isNull"))
ExprCode(
code"""
boolean $isNull = $column.isNullAt(rowId);
$javaType $value = $isNull ? ${CodeGenerator.defaultValue(attr.dataType)} : ($getter);
""",
isNull,
value)
} else {
ExprCode(code"$javaType $value = $getter;", FalseLiteral, value)
}
}
val projection = GenerateUnsafeProjection.createCode(ctx, fields)
val bindColumns = columns.zipWithIndex
.map { case (column, i) =>
s"$column = batch.column($i);"
}
.mkString("\n")
val code = s"""
public Object generate(Object[] references) {
return new SpecificCachedBatchRowIterator((scala.collection.Iterator) references[0]);
}

class SpecificCachedBatchRowIterator extends scala.collection.AbstractIterator {
private final scala.collection.Iterator batches;
private int rowId = 0;
private int numRows = 0;
${ctx.declareMutableStates()}

public SpecificCachedBatchRowIterator(scala.collection.Iterator batches) {
this.batches = batches;
${ctx.initMutableStates()}
}

public boolean hasNext() {
while (rowId >= numRows && batches.hasNext()) {
${classOf[ColumnarBatch].getName} batch =
(${classOf[ColumnarBatch].getName}) batches.next();
numRows = batch.numRows();
rowId = 0;
$bindColumns
}
return rowId < numRows;
}

public InternalRow next() {
if (!hasNext()) throw new java.util.NoSuchElementException();
${projection.code}
rowId++;
return ${projection.value};
}

${ctx.declareAddedFunctions()}
}
"""
val (compiled, _) =
CodeGenerator.compile(new CodeAndComment(code, ctx.getPlaceHolderToComments()))
compiled.generate(Array[Any](batches)).asInstanceOf[Iterator[InternalRow]]

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.

The generated code hard-codes references[0] as the batch iterator while ctx.references starts empty. That holds today because nothing in GenerateUnsafeProjection.createCode for BoundReferences adds a reference, but if that ever changes index 0 would silently become something else. Would you consider registering the iterator with ctx.addReferenceObj("batches", batches) and passing ctx.references.toArray to generate, the way Spark's own generators do?

}

override protected def createInterpretedObject(
batches: Iterator[ColumnarBatch]): Iterator[InternalRow] = {
val toUnsafe = InterpretedUnsafeProjection.createProjection(fields)
batches.flatMap { batch =>
new Iterator[InternalRow] {
private var rowId = 0
override def hasNext: Boolean = rowId < batch.numRows()
override def next(): InternalRow = {
if (!hasNext) throw new NoSuchElementException
val row = toUnsafe(batch.getRow(rowId))
rowId += 1
row
}
}
}
}
}
Loading
Loading