dongjoon-hyun commented on PR #58605:
URL: https://github.com/apache/spark/pull/58605#issuecomment-5584087922
Thanks for the patch. A few comments, roughly in order of importance.
### 1. There are no tests
`How was this patch tested?` only lists `hive/compile` and
`sql-kafka-0-10/compile`. Two new `SQLConf` entries and the rejection paths
they gate should come with tests, at minimum:
- `KafkaSourceProviderSuite` / `KafkaRelationSuite`: a listed option is
rejected on both the source and the sink path.
- `AvroSuite`: allowed / rejected schemes for `avroSchemaUrl`.
- For both: the empty-default case still behaves exactly as before.
### 2. With `spark.sql.avro.schemaUrlAllowedSchemes` set, a scheme-less path
can never be allowed
In `AvroOptions`, a `null` scheme is mapped to `""` and then looked up in
the allowlist. But `stringConf.toSequence` goes through
`SparkStringUtils.stringToSeq`, which does `.filter(_.nonEmpty)`, so an empty
entry cannot be put into the list in the first place.
That means once the allowlist is enabled, `avroSchemaUrl =
"/schemas/user.avsc"` (a path resolved against `fs.defaultFS`, which is the
common form in practice) always fails with
```
The scheme '' of avroSchemaUrl '/schemas/user.avsc' is not in the allowlist
...
```
with no way to permit it. Either resolve the URL against the default FS
before checking the scheme, or make the scheme-less case explicitly allowed.
### 3. The Hive change is not opt-in, and it is user-facing
`Does this PR introduce any user-facing change? -> No by default` does not
hold for the `HiveClientImpl` change: it applies unconditionally. For an
`InputFormat`/`OutputFormat` whose static initializer fails, the failure moves
from DDL time (`CREATE`/`ALTER TABLE`) to scan/write time. Worth correcting the
description, and possibly a migration-guide note.
(The change itself looks safe -- the returned `Class` is only consumed by
`hiveTable.setInputFormatClass`, which just stores `getName()`.)
### 4. The description mentions serde, but the diff does not touch it
> Hive metastore serde/InputFormat class names are resolved with `initialize
= false`
Only `toInputFormat` / `toOutputFormat` are changed. The serde goes through
`setSerializationLib(String)`, so no class loading is involved there. Please
drop `serde` from the description.
### 5. Consider splitting this into separate PRs
The Kafka option denylist, the Avro URL scheme allowlist, and the Hive
class-initialization change are unrelated -- different modules, and the first
two are opt-in controls while the third is an unconditional class-loading
behavior change. Separate JIRAs/PRs would be easier to review and to revert
independently.
### Minor
- **Use error classes instead of raw `IllegalArgumentException`.**
`AvroOptions` already has `QueryCompilationErrors.avroOptionsException` for
option errors, and the Kafka connector has `KafkaExceptions` with its own
error-class JSON.
- **`disallowed.nonEmpty &&` is redundant** -- `contains` on an empty `Set`
is already `false`.
- **Throwing inside `map` reads oddly.** This is validation, not a
transformation. Splitting it out is clearer:
```scala
val stripped = parameters.keySet
.filter(_.toLowerCase(Locale.ROOT).startsWith("kafka."))
.map { k => k.drop(6) -> parameters(k) }
stripped.foreach { case (key, _) =>
if (disallowed.contains(key.toLowerCase(Locale.ROOT))) throw ...
}
stripped.toMap
```
- **`new URI(url)` is built twice** in `AvroOptions` (once for the scheme
check, once for `FileSystem.get`). Also note the check parses with `URI` while
the open uses `new Path(url)`, i.e. two different parsers.
- **Config placement / namespace.** `KAFKA_DISALLOWED_OPTIONS` currently
sits between the Avro and JSON configs; moving it next to the existing
`USE_DEPRECATED_KAFKA_OFFSET_FETCHING` would fit the file's grouping better.
Also, the existing Kafka config namespace is `spark.sql.streaming.kafka.*` --
is opening a new `spark.sql.kafka.*` namespace intended? (Dropping `streaming.`
does seem right since this applies to batch too.)
### Looks fine
- `convertToSpecifiedParams` is the only place the `kafka.` prefix is
stripped, so the denylist has no gaps across the source/sink/batch/streaming
call sites.
- All call sites are driver-side, so `SQLConf.get` is safe in both the Kafka
and the Avro change.
- `withBindingPolicy(NOT_APPLICABLE)` looks like the right choice for both
configs per the decision rules in `ConfigBindingPolicy`.
--
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]
