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 @@ -579,6 +579,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 @@ -227,6 +227,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
33 changes: 19 additions & 14 deletions spark/src/main/scala/org/apache/comet/CometConf.scala
Original file line number Diff line number Diff line change
Expand Up @@ -263,20 +263,25 @@ object CometConf extends ShimCometConf {
val COMET_EXEC_IN_MEMORY_CACHE_ENABLED: ConfigEntry[Boolean] =
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 " +
"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 " +
"format. Each cached batch is stored as one Arrow IPC record batch with per-buffer " +
"zstd compression, and a scan copies out only the buffers of the columns it projected, " +
"so the unselected ones are never decompressed. 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 " +
"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.")
.doc(
"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 " +
"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 batch is stored as one Arrow IPC record batch with per-buffer " +
"zstd compression, and a scan copies out only the buffers of the columns it " +
"projected, so the unselected ones are never decompressed. 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 " +
Comment on lines +277 to +280

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.

This describes the new split, but the Limitations section of docs/source/user-guide/latest/in-memory-cache.md still says Spark-operator reads are slower than Spark's own format and that the cause is not yet established. Its table was measured with Comet off, and those reads now go through CachedBatchRowIterator. With Comet on and exec off, eligible consumers take the fused path instead. Could you update that section in this PR, including when the fused path applies? With exec on, a Spark operator above the cache already reads through CometColumnarToRow over the native scan, so that case does not change.

"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.")
.booleanConf
.createWithDefault(false)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: CometRule (no-op, already converted)
* b. insertTransitions
* c. postColumnarTransitions: RevertNativeForTransitionHeavyStages,
* EliminateRedundantTransitions
* EliminateRedundantTransitions, CometCacheColumnarRule
* }}}
*
* On Spark 3.4, injectQueryStageOptimizerRule is unavailable. CometExecRule does not wrap SABs,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/*
* 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.spark.sql.internal.SQLConf

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

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.

Since #5394, CometColumnar.postColumnarTransitions runs these rules in plan-only mode too, and this rule rewrites plans that contain no Comet operators. With spark.comet.explain.planOnly.enabled=true, a read of a Comet-format cache executes a ColumnarToRowExec that Spark would not have planned, while the plan-only doc says Spark executes the query unchanged. Should this also return early when plan-only is on? A plan-only case in the runtime-settings test would pin it down.

if (!conf.wholeStageEnabled) return plan
Comment thread
peterxcli marked this conversation as resolved.
if (conf.getConf(SQLConf.CODEGEN_FACTORY_MODE).toString == "NO_CODEGEN") return plan

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
})
}
}
3 changes: 2 additions & 1 deletion spark/src/main/scala/org/apache/comet/rules/CometRule.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ object CometRule {
def postColumnarRules(session: SparkSession, wholePlan: Boolean = false): Seq[Rule[SparkPlan]] =
Seq(
RevertNativeForTransitionHeavyStages(session, wholePlan),
EliminateRedundantTransitions(session))
EliminateRedundantTransitions(session),
CometCacheColumnarRule)

/**
* Canonical hashes of the subquery plans reported for the query this thread is preparing. Spark
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import scala.collection.JavaConverters._
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, StartsWith, UnsafeProjection}
import org.apache.spark.sql.catalyst.expressions.{Attribute, Expression, GenericInternalRow, IsNotNull, IsNull, StartsWith}
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 @@ -696,11 +696,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,131 @@
/*
* 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)

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.

[P2] Preserve code splitting for wide cache projections. Reading 1,500 selected INT columns with alternating nullable/non-nullable attributes makes the generated next() exceed the JVM's 64 KB method limit. Setting ctx.currentVars above causes GenerateUnsafeProjection to skip its normal splitExpressions path, whereas the previous UnsafeProjection.create(...) reader compiles successfully for the same schema. Under default FALLBACK, each partition retries the failed compilation and then uses the interpreted reader. The local probe measured later constructions at 400–515 ms versus 43–63 ms for the previous path, with correct results from both. CODEGEN_ONLY fails outright. Could this split the generated writer into bounded methods, or select the existing generated UnsafeProjection path for wide schemas before attempting oversized compilation? A 1,500-column regression case would cover this.

Evidence: Compiled the unchanged exact-head CachedBatchRowIterator.scala against Spark 4.1.3 with Scala 2.13.17/JDK 21. A package-local harness created attributes using (0 until 1500).map(i => AttributeReference(s"c$i", IntegerType, nullable = i % 2 == 0)()) and matching OnHeapColumnVectors. With CODEGEN_ONLY, the previous UnsafeProjection.create(attrs, attrs) path passed, while new CachedBatchRowIterator(attrs).createObject(Iterator.single(batch)) failed with InternalCompilerException: Code grows beyond 64 KB while compiling next(). Widths 150, 500 and 1,000 passed. In a separate default-FALLBACK probe with 4,096 rows and logging disabled, five alternating old/new runs returned identical checksums. The final three construction times were 62.95/45.58/43.28 ms for the previous path and 399.82/484.91/514.83 ms for the new interpreted fallback. Spark's GenerateUnsafeProjection.writeExpressionsToBuffer explicitly bypasses splitting when ctx.currentVars != null. Harnesses and logs are under /tmp/comet-5859-dbfb-probe/.

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.

This starts well below the 64 KB limit. Under it the method still compiles, but once next() passes HotSpot's 8000-byte HugeMethodLimit the JIT never compiles it and it stays interpreted. I timed this iterator against main's UnsafeProjection plus copy() over nullable bigint columns, 8 batches of 4096 rows each. It is 0.39x at 6 columns and about the same as main at 50. It is 1.7x slower at 100 columns and 13 to 15x slower from 120 to 500. String columns reach 4x at 150. With -XX:-DontCompileHugeMethods the 120 and 150 column cases drop back to about 1.2x, which points at the method size. End to end, reading a 150-column Comet cache took 320 ms on this PR against 66 ms on the merge base with Comet on and exec off, and 323 ms against 64 ms with Comet off. Spark reads of relations wider than spark.sql.codegen.maxFields always take this path, because InMemoryTableScanExec.supportsColumnar is false for them.

So falling back only when compilation fails would not be enough. CodeGenerator.compile returns the ByteCodeStats that line 110 discards, and that is what WholeStageCodegenExec checks before it backs off. A width bound would work too. For the fallback itself, UnsafeProjection.create(fields) over batch.getRow(i) in an indexed loop, without the copy(), came in at 0.64 to 0.93x of main at every width I tried. Could the benchmark and the tests cover 100 and 200 columns as well as 1,500?

val batchesRef = ctx.addReferenceObj("batches", batches, "scala.collection.Iterator")
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(references);
}

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

public SpecificCachedBatchRowIterator(Object[] references) {
this.references = references;
this.batches = $batchesRef;
${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(ctx.references.toArray).asInstanceOf[Iterator[InternalRow]]
}

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