Skip to content

Commit 0d95cc4

Browse files
committed
use filter, don't assert at all if no parquet encryptions requried
1 parent 12b7f83 commit 0d95cc4

2 files changed

Lines changed: 25 additions & 20 deletions

File tree

spark/src/main/scala/org/apache/spark/sql/comet/operators.scala

Lines changed: 22 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -217,24 +217,14 @@ abstract class CometNativeExec extends CometExec {
217217
// TODO: support native metrics for all operators.
218218
val nativeMetrics = CometMetricNode.fromCometPlan(this)
219219

220+
// Go over all all the native scans, in order to see if they need encryption options.
220221
// For each relation in a CometNativeScan generate a hadoopConf,
221222
// for each file path in a relation associate with hadoopConf
222223
// This is done per native plan, so only count scans until a comet input is reached.
223-
val cometNativeScans = mutable.ArrayBuffer.empty[CometNativeScanExec]
224+
val encryptionOptions =
225+
mutable.ArrayBuffer.empty[(Broadcast[SerializableConfiguration], Seq[String])]
224226
foreachUntilCometInput(this) {
225-
case scan: CometNativeScanExec => cometNativeScans += scan
226-
case _ => // no-op
227-
}
228-
assert(
229-
cometNativeScans.size <= 1,
230-
"We expect one native scan in a Comet plan since we will broadcast one hadoopConf.")
231-
// If this assumption changes in the future, you can look at the commit history of #2447
232-
// to see how there used to be a map of relations to broadcasted confs in case multiple
233-
// relations in a single plan. The example that came up was UNION. See discussion at:
234-
// https://github.com/apache/datafusion-comet/pull/2447#discussion_r2406118264
235-
val (broadcastedHadoopConfForEncryption, encryptedFilePaths) =
236-
cometNativeScans.headOption.fold(
237-
(None: Option[Broadcast[SerializableConfiguration]], Seq.empty[String])) { scan =>
227+
case scan: CometNativeScanExec =>
238228
// This creates a hadoopConf that brings in any SQLConf "spark.hadoop.*" configs and
239229
// per-relation configs since different tables might have different decryption
240230
// properties.
@@ -246,10 +236,25 @@ abstract class CometNativeExec extends CometExec {
246236
val broadcastedConf =
247237
scan.relation.sparkSession.sparkContext
248238
.broadcast(new SerializableConfiguration(hadoopConf))
249-
(Some(broadcastedConf), scan.relation.inputFiles.toSeq)
250-
} else {
251-
(None, Seq.empty)
239+
240+
val optsTuple: (Broadcast[SerializableConfiguration], Seq[String]) =
241+
(broadcastedConf, scan.relation.inputFiles.toSeq)
242+
encryptionOptions += optsTuple
252243
}
244+
case _ => // no-op
245+
}
246+
assert(
247+
encryptionOptions.size <= 1,
248+
"We expect one native scan that requires encryption reading in a Comet plan," +
249+
" since we will broadcast one hadoopConf.")
250+
// If this assumption changes in the future, you can look at the commit history of #2447
251+
// to see how there used to be a map of relations to broadcasted confs in case multiple
252+
// relations in a single plan. The example that came up was UNION. See discussion at:
253+
// https://github.com/apache/datafusion-comet/pull/2447#discussion_r2406118264
254+
val (broadcastedHadoopConfForEncryption, encryptedFilePaths) =
255+
encryptionOptions.headOption match {
256+
case Some((conf, paths)) => (Some(conf), paths)
257+
case None => (None, Seq.empty)
253258
}
254259

255260
def createCometExecIter(

spark/src/test/scala/org/apache/spark/sql/comet/ParquetEncryptionITCase.scala

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -398,12 +398,12 @@ class ParquetEncryptionITCase extends CometTestBase with SQLTestUtils {
398398
// If we add another comet aggregate after the union, we see the need for the
399399
// foreachUntilCometInput() in operator.scala
400400
// as we would error on multiple native scan execs despite no longer being in the same plan at all
401-
val aggDf = unionDF.agg(functions.sum("id"))
401+
val filterDf = unionDF.filter(functions.col("id") % 2 === 0)
402402

403403
if (CometConf.COMET_ENABLED.get(conf)) {
404-
checkSparkAnswerAndOperator(aggDf)
404+
checkSparkAnswerAndOperator(filterDf)
405405
} else {
406-
checkSparkAnswer(aggDf)
406+
checkSparkAnswer(filterDf)
407407
}
408408
}
409409
}

0 commit comments

Comments
 (0)