andygrove commented on code in PR #5859: URL: https://github.com/apache/datafusion-comet/pull/5859#discussion_r3999884396
########## spark/src/main/scala/org/apache/comet/rules/CometCacheColumnarRule.scala: ########## @@ -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 Review Comment: 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. ########## spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/CachedBatchRowIterator.scala: ########## @@ -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]] Review Comment: 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 `BoundReference`s 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? ########## spark/src/test/scala/org/apache/spark/sql/benchmark/CometCacheRowReaderBenchmark.scala: ########## @@ -0,0 +1,204 @@ +/* + * 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.benchmark + +import java.nio.charset.StandardCharsets + +import org.apache.spark.benchmark.BenchmarkBase +import org.apache.spark.sql.{DataFrame, Row, SparkSession} +import org.apache.spark.sql.comet.execution.arrow.ArrowCachedBatchSerializer +import org.apache.spark.sql.execution.ColumnarToRowExec +import org.apache.spark.sql.execution.columnar.{CometInMemoryRelationHelper, DefaultCachedBatch, DefaultCachedBatchSerializer, InMemoryRelation, InMemoryTableScanExec} +import org.apache.spark.sql.internal.SQLConf +import org.apache.spark.storage.StorageLevel + +import org.apache.comet.{CometConf, CometSparkSessionExtensions} + +/** + * Compare Spark consumers of Comet and Spark caches (issue #5485). + * + * Arguments: [spark|comet|comet-row|all] [rows] [iterations] [all|mixed|numeric]. Run one format + * per JVM in alternating order on main and the patch. comet-row disables vectorized cache reading + * to isolate the row iterator. Cache creation and validation are outside timing. + */ +object CometCacheRowReaderBenchmark extends BenchmarkBase { + private val warmups = 5 + + override def runBenchmarkSuite(args: Array[String]): Unit = { + require(args.length <= 4, "Expected format, rows, iterations, schema") + val format = args.headOption.getOrElse("all") + val rows = args.lift(1).map(_.toLong).getOrElse(5000000L) + val iterations = args.lift(2).map(_.toInt).getOrElse(15) + val schema = args.lift(3).getOrElse("all") + require(Set("all", "spark", "comet", "comet-row").contains(format)) + require(Set("all", "mixed", "numeric").contains(schema)) + require(rows > 0 && iterations > 0) + + emit("CACHE_SAMPLE,format,schema,query,rows,iteration,elapsed_ns") + val formats = + if (format == "all") Seq("spark", "comet", "comet-row") else Seq(format) + val schemas = if (schema == "all") Seq("mixed", "numeric") else Seq(schema) + formats.foreach { name => + CometInMemoryRelationHelper.clearSerializer() + SparkSession.clearActiveSession() + SparkSession.clearDefaultSession() + val serializer = if (name == "spark") { + classOf[DefaultCachedBatchSerializer].getName + } else { + classOf[ArrowCachedBatchSerializer].getName + } + val spark = SparkSession + .builder() + .master("local[1]") + .appName(getClass.getSimpleName) + .config("spark.ui.enabled", "false") + .config("spark.sql.cache.serializer", serializer) + .config("spark.sql.shuffle.partitions", "1") + .config("spark.sql.inMemoryColumnarStorage.batchSize", "10000") + .config("spark.sql.inMemoryColumnarStorage.compressed", "true") + .config("spark.io.compression.codec", "lz4") + .config(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key, "false") + .config(SQLConf.WHOLESTAGE_CODEGEN_ENABLED.key, "true") + .config(SQLConf.CACHE_VECTORIZED_READER_ENABLED.key, (name != "comet-row").toString) + .config(SQLConf.CODEGEN_FACTORY_MODE.key, "CODEGEN_ONLY") + .config(CometConf.COMET_ENABLED.key, "true") + .config(CometConf.COMET_EXEC_IN_MEMORY_CACHE_ENABLED.key, "true") + .config(CometConf.COMET_EXEC_ENABLED.key, "false") + .config(CometConf.COMET_SHUFFLE_ENABLED.key, "false") + .withExtensions(new CometSparkSessionExtensions) + .getOrCreate() + spark.sparkContext.setLogLevel("WARN") + try { + emit(s"CACHE_ENV,$name,Spark=${spark.version},Java=${System.getProperty("java.version")}") + schemas.foreach(runSchema(spark, name, _, rows, iterations, serializer)) + } finally { + spark.stop() + SparkSession.clearActiveSession() + SparkSession.clearDefaultSession() + CometInMemoryRelationHelper.clearSerializer() + } + } + } + + private def runSchema( + spark: SparkSession, + format: String, + schema: String, + rows: Long, + iterations: Int, + serializer: String): Unit = { + val mixed = schema == "mixed" + val first = Seq("id", "id % 1000 AS k", "id + 1 AS v") + val rest = if (mixed) { + Seq( + "concat('str_a_', cast(id % 100000 as string)) AS s1", + "concat('str_b_', cast(id % 7919 as string)) AS s2", + "concat('str_c_', cast(id as string)) AS s3") + } else { + Seq("id % 100000 AS n1", "id % 7919 AS n2", "id * 3 AS n3") + } + val source = spark.range(0, rows, 1, 16).selectExpr((first ++ rest): _*) + val columns = source.columns.toSeq + val three = if (mixed) Seq("id", "s1", "s2") else columns.take(3) + val projections = Seq("count" -> Seq.empty[String], "long" -> Seq("id")) ++ + (if (mixed) Seq("string" -> Seq("s1")) else Seq.empty) ++ + Seq("three" -> three, "all" -> columns) + def expressions(selected: Seq[String]): Seq[String] = { + if (selected.isEmpty) Seq("count(*)") + else + selected.map { name => + if (name.startsWith("s")) s"sum(length($name))" else s"sum($name)" + } + } + // Obtain the expected values before the relation is cached, using Spark's ordinary row plan. + val expected = projections.map { case (_, selected) => + source.selectExpr(expressions(selected): _*).collect() + } + val cached = source.persist(StorageLevel.MEMORY_ONLY) + try { + assert(cached.count() == rows) + val relation = cached.queryExecution.withCachedData.collectFirst { + case relation: InMemoryRelation => relation + }.get + val builder = relation.cacheBuilder + assert(builder.serializer.getClass.getName == serializer) + val batches = builder.cachedColumnBuffers + val batchSummary = batches + .map { batch => + // Spark's sizeInBytes comes from statistics; measure its encoded column buffers. + val bytes = batch match { + case b: DefaultCachedBatch => b.buffers.map(_.length.toLong).sum + case _ => batch.sizeInBytes + } + (batch.getClass.getSimpleName, batch.numRows.toLong, bytes) + } + .collect() + val expectedClass = if (format == "spark") "DefaultCachedBatch" else "CometCachedBatch" + assert(batchSummary.forall(_._1 == expectedClass), "Wrong cached payload format") + assert(batchSummary.map(_._2).sum == rows) + val storage = spark.sparkContext.getRDDStorageInfo.find(_.id == batches.id).get + assert(storage.numCachedPartitions == batches.getNumPartitions && storage.diskSize == 0) + emit( + s"CACHE_STORAGE,$format,$schema,${batchSummary.length}," + + s"${batchSummary.map(_._3).sum},${storage.memSize}") + + projections.zip(expected).foreach { case ((name, selected), answer) => + val query = cached.selectExpr(expressions(selected): _*) + val plan = query.queryExecution.executedPlan + val scans = plan.collect { case scan: InMemoryTableScanExec => scan } + assert(scans.size == 1, s"Expected one Spark cache scan:\n$plan") + val scan = scans.head + assert(scan.attributes.map(_.name).toSet == selected.toSet, s"Wrong projection:\n$plan") + val columnar = plan.exists(_.isInstanceOf[ColumnarToRowExec]) + if (format != "comet") { Review Comment: This asserts the row reader for `spark` and `comet-row` but never asserts the columnar reader for `comet`. If the rule stops firing for any reason, the `comet` column of the results table would quietly measure the row path. Could you add `assert(columnar)` for the `comet` format so the benchmark fails loudly instead? -- 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]
