HyukjinKwon commented on code in PR #58614:
URL: https://github.com/apache/spark/pull/58614#discussion_r4004067690
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##########
@@ -7016,6 +7016,22 @@ object SQLConf {
.booleanConf
.createWithDefault(true)
+ // Kept here with the other `spark.sql.avro.*` entries rather than in
`StaticSQLConf`, though it
+ // is static: `buildStaticConf` registers the static key wherever it is
declared.
+ val AVRO_SCHEMA_URL_ALLOWED_SCHEMES =
+ buildStaticConf("spark.sql.avro.schemaUrlAllowedSchemes")
+ .internal()
+ .doc("A comma-separated allowlist of URI schemes permitted for the
'avroSchemaUrl' Avro " +
+ "option. Empty by default, which permits any scheme and preserves the
previous behavior; " +
+ "when non-empty, an avroSchemaUrl whose scheme is not listed is
rejected before it is " +
+ "opened. This is a static configuration fixed when the SparkSession is
created and not " +
+ "modifiable at runtime, so it is an operator-level boundary that a
session cannot relax.")
Review Comment:
Fixed. Took your suggested framing: the `.doc` (d325505) now says the
allowlist restricts the scheme an `avroSchemaUrl` may name, not which file
system serves it -- that is decided by `fs.<scheme>.impl`, which a session can
still set. Dropped the "a session cannot relax it" claim from both the doc and
the PR description. Separately, per finding 6, I moved the conf into
`StaticSQLConf` next to the other 4.3.0 static allowlists (cb6ac21);
`AvroSchemaUrlAllowlistSuite`'s `CANNOT_MODIFY_STATIC_CONFIG` case confirms the
static registration still holds.
##########
connector/avro/src/test/scala/org/apache/spark/sql/avro/AvroSuite.scala:
##########
@@ -1255,6 +1255,91 @@ 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.
+ // testFile returns a "file:" URL, so its scheme is an explicit "file"; the
scheme-less path that
+ // resolves against the default file system is covered by its own test below.
+ 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. The URL uses an
+ // upper-case "S3A" scheme so the lower-case "s3a" in the message pins the
scheme-side case
+ // folding: dropping the fold on the scheme leaves no lower-case "s3a" in
the message.
+ 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)
+ }
+ assert(e.getCondition == "STDS_INVALID_OPTION_VALUE.WITH_MESSAGE")
+ assert(e.getMessage.contains("avroSchemaUrl"))
+ assert(e.getMessage.contains("not in the allowlist"))
+ assert(e.getMessage.contains("The scheme 's3a'"))
Review Comment:
Added in d325505: `AvroSuite` "SPARK-59329: the allowlist rejection echoes
the parsed allowlist" uses a `file://` entry (which parses to one entry that
matches nothing) and asserts the message contains both `The scheme 'file'` and
`not in the allowlist [file://]`, so dropping the parsed-allowlist echo fails
it. Runs green on head; the second assertion is exactly what pins the mutation
you measured.
--
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]
