peter-toth commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r3987277354
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does
not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it
cannot be set with
Review Comment:
**Finding 5.** The static conf does rule out `withSQLConf`, but not a read.
These four tests hand a hand-built `SQLConf` to the check through
`withExistingConf`, which is the one input that cannot be wrong in production.
So the three properties the round-2 change is *for* are all unpinned:
- that the conf set on the `SparkSession` is visible at the `SQLConf.get`
the check reads,
- that the check fires in a real `spark.read ... load()`,
- that a session cannot relax it, which is what `buildStaticConf` bought.
All three hold on this head - I ran them - so this is coverage, not a bug. A
dedicated suite with `override protected def sparkConf` gets them, the same
pattern `AvroV1Suite` uses at
`connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:3925`.
This compiles and passes as written:
```scala
class AvroSchemaUrlAllowlistSuite extends QueryTest with SharedSparkSession {
override protected def sparkConf: SparkConf =
super.sparkConf.set(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key, "file")
private val testAvro = testFile("test.avro")
test("SPARK-59329: an allowed scheme is permitted through a read") {
val result = spark.read.option("avroSchemaUrl",
testFile("test_sub.avsc"))
.format("avro").load(testAvro).collect()
val expected =
spark.read.format("avro").load(testAvro).select("string").collect()
assert(result.sameElements(expected))
}
test("SPARK-59329: a disallowed scheme is rejected through a read") {
val e = intercept[AnalysisException] {
spark.read.option("avroSchemaUrl", "s3a://bucket/user.avsc")
.format("avro").load(testAvro).collect()
}
assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
assert(e.getMessage.contains("not in the allowlist"))
}
test("SPARK-59329: a session cannot relax the allowlist") {
val key = SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES.key
Seq[() => Unit](
() => spark.conf.set(key, "s3a"),
() => spark.sql(s"SET $key=s3a").collect()
).foreach { f =>
checkError(
exception = intercept[AnalysisException](f()),
condition = "CANNOT_MODIFY_STATIC_CONFIG",
parameters = Map("key" -> s""""$key""""))
}
}
}
```
Keep whichever of the direct-`AvroOptions` cases you still want in
`AvroSuite` on top of that.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,64 @@ abstract class AvroSuite
assertExceptionMsg[FileNotFoundException](e, "File not_exists.avsc does
not exist")
}
+ // spark.sql.avro.schemaUrlAllowedSchemes is a static SQL config, so it
cannot be set with
+ // withSQLConf; these drive AvroOptions directly under a SQLConf provided
via withExistingConf.
+ // A scheme-less local path resolves to the default file system ("file").
+ test("SPARK-59329: avroSchemaUrl scheme allowlist permits an allowed
scheme") {
+ val avroSchemaUrl = testFile("test_sub.avsc")
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val options = new AvroOptions(Map("avroSchemaUrl" -> avroSchemaUrl),
hadoopConf)
+ assert(options.schema.isDefined)
+ }
+ }
+
+ test("SPARK-59329: avroSchemaUrl allowlist rejects a disallowed scheme " +
+ "before opening the file system") {
+ // An explicit non-"file" scheme is rejected by the allowlist check, which
runs before the
+ // file system for the URL is instantiated -- so this surfaces the clean
allowlist error
+ // rather than a lower-level failure from trying to load the s3a file
system.
+ val hadoopConf = spark.sessionState.newHadoopConf()
+ val conf = new SQLConf()
+ conf.setConf(SQLConf.AVRO_SCHEMA_URL_ALLOWED_SCHEMES, Seq("file"))
+ SQLConf.withExistingConf(conf) {
+ val e = intercept[AnalysisException] {
+ new AvroOptions(Map("avroSchemaUrl" -> "s3a://bucket/user.avsc"),
hadoopConf)
Review Comment:
**Finding 8.** The new test's comment says the entries and the URL scheme
are both folded, but only the entry side is pinned - that half was finding 2. I
deleted the `.toLowerCase(Locale.ROOT)` on the scheme at
`sql/core/src/main/scala/org/apache/spark/sql/avro/AvroOptions.scala:93` and
all four SPARK-59329 tests still passed. Every URL in them is scheme-less, so
the scheme always arrives from `FileSystem.getDefaultUri` already lower-case.
One character here covers it. With an upper-case scheme the
`assert(e.getMessage.contains("s3a"))` three lines below becomes the pin: the
message reads `The scheme 's3a' of avroSchemaUrl 'S3A://bucket/user.avsc'` with
the fold, and without it there is no lower-case `s3a` left anywhere in it. I
ran it both ways - passes as the code stands, fails with the fold removed.
```suggestion
new AvroOptions(Map("avroSchemaUrl" -> "S3A://bucket/user.avsc"),
hadoopConf)
```
Tightening that assertion to `contains("The scheme 's3a'")` makes the intent
explicit if you prefer.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,20 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
Review Comment:
**Finding 6.** This is the only `buildStaticConf` in `SQLConf.scala`. The
other 39 SQL static confs live in `StaticSQLConf.scala`, and Hive's and
Connect's live in their own module objects (`HiveUtils`, `Connect`).
`StaticSQLConf` is where a reader goes to find out what cannot be set at
runtime. Here the entry sits between `spark.sql.avro.filterPushdown.enabled`
and `spark.sql.json.enablePartialResults`, with nothing but the builder name
marking it apart from the runtime confs around it.
Moving it to `StaticSQLConf.scala` is the smaller surprise. If you would
rather keep it with the other `spark.sql.avro.*` entries, a one-line comment
above it saying it is static would do.
--
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]
