jackylee-ch commented on code in PR #12549:
URL: https://github.com/apache/gluten/pull/12549#discussion_r3711812308
##########
gluten-substrait/src/main/scala/org/apache/spark/shuffle/GlutenShuffleUtils.scala:
##########
@@ -49,42 +48,36 @@ object GlutenShuffleUtils {
}
def getCompressionCodec(conf: SparkConf): String = {
- def checkCodecValues(codecConf: String, codec: String, validValues:
Set[String]): Unit = {
+ // Gluten's codec conf falls back to Spark's spark.io.compression.codec,
so reading it always
+ // yields a value. Look both keys up in SQLConf first and then in the
given SparkConf, matching
+ // how a session inherits from the application conf.
+ val provider =
+ new ChainedProvider(new SQLConfProvider(SQLConf.get), new
SparkConfProvider(conf))
+ val codecEntry = GlutenConfig.COLUMNAR_SHUFFLE_CODEC
+ val (codec, isSetOnGlutenConf) = codecEntry.readWithSource(provider)
+ val supportedCodecs =
BackendsApiManager.getSettings.shuffleSupportedCodec()
+ if (isSetOnGlutenConf) {
+ // An explicitly set codec is validated against the codec backend in use.
+ val validValues = if (GlutenConfig.get.columnarShuffleEnableQat) {
+ GlutenConfig.GLUTEN_QAT_SUPPORTED_CODEC
+ } else {
+ supportedCodecs
+ }
if (!validValues.contains(codec)) {
throw new IllegalArgumentException(
- s"The value of $codecConf should be one of " +
+ s"The value of ${codecEntry.key} should be one of " +
s"${validValues.mkString(", ")}, but was $codec")
}
Review Comment:
Sorted both, in `acf36647b`.
On the nondeterminism itself: for the current inputs the ordering is in fact
stable — every one of these sets has ≤ 4 elements, so Scala uses the
specialized `Set2`/`Set3` classes, which iterate in insertion order rather than
by hash. `Set("lz4", "zstd")` (Velox), `Set("lz4", "zstd", "snappy")`
(ClickHouse) and `Set("gzip", "zstd")` (QAT) therefore already print
deterministically, and stay stable across JVMs. The risk is real but latent: it
would surface the moment a backend adds a 5th codec and the value becomes a
`HashSet`.
Sorting is worth it anyway, for the reason the comment gives second — a
stable, alphabetical list is easier to read and to assert on — so both call
sites now use `.toSeq.sorted.mkString(...)`. Note this changes no existing
message: `lz4, zstd` and `lz4 or zstd` happen to already be in sorted order.
Two related notes:
- The `mkString` in `TypedConfigBuilder.checkValues`
(`ConfigBuilder.scala:319`) has the same latent issue but is pre-existing and
untouched by this PR, so I left it out to keep the diff scoped. Happy to sort
it in a follow-up.
- The three `MiscOperatorSuite` codec tests assert on substrings (`does not
support codec 'snappy'`, `spark.shuffle.compress=false`, the conf key), not on
the codec list, so they are unaffected.
--
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]
