dongjoon-hyun commented on PR #58597:
URL: https://github.com/apache/spark/pull/58597#issuecomment-5581099323
Thanks for working on this. I went through the diff and the CI results; a
few things below.
## Blockers
**1. `ML_MAX_NUM_FEATURES` is missing `.withBindingPolicy(...)`, which is
breaking CI.**
`Run / Build modules: hive - other tests` is failing on
`SparkConfigBindingPolicySuite`, test `Config enforcement for bindingPolicy`.
All eight recently-added entries in `StaticSQLConf` declare a binding policy,
and `spark.sql.ml.maxNumFeatures` is not in the exceptions file (which is
frozen and may only shrink). Following the convention used by the
`spark.sql.pythonWorkerEnv.*` entries right below:
```scala
.version("4.4.0")
.internal()
.withBindingPolicy(ConfigBindingPolicy.NOT_APPLICABLE)
.intConf
.checkValue(v => v == -1 || v > 0, "Must be -1 (disabled) or a positive
value.")
.createWithDefault(-1)
```
I'd add the `.checkValue` too: today `0` or `-7` are silently accepted and
behave as "disabled", since the guard in `check` is `max > 0`.
**2. `.version("4.3.0")` is wrong.**
```
$ dev/next_version_candidates.py
master 5.0.0
branch-4.x 4.4.0
```
This is a normally-backported change against `master`, so it first ships in
`branch-4.x`, i.e. `4.4.0`. The neighboring entries in the same file already
use `4.4.0`.
(The `KeyGroupedPartitioningSuite` `SPARK-59050` failure is unrelated -- it
was fixed on master by `f957459c9ae`; a rebase will clear it.)
## Design
**3. It is declared as a static SQL conf but never read through `SQLConf`.**
`MLMaxNumFeatures` reads `SparkEnv.get.conf`, which is a plain `SparkConf`
lookup. So putting the entry in `StaticSQLConf` (sql/catalyst) buys only
`SparkSession.conf` visibility and `SET` rejection, at the cost of adding an ML
concept to the SQL config surface. A regular `ConfigEntry` under
`org.apache.spark.internal.config` would work with the current implementation
unchanged. Also, `spark.sql.ml.*` is a new namespace with no precedent in the
repo.
**4. `.internal()` vs. "user-facing change: Yes".**
The description says yes, but the entry is internal (so it will not be
documented). If operators are meant to turn this on, please drop `.internal()`
and document it; otherwise the description should say no.
**5. Only one of the three sites actually performs an allocation
proportional to the declared count.**
- `AttributeGroup.fromMetadata` does `new Array[Attribute](numAttrs)` --
this is the real one (up to ~17 GB of references at `Int.MaxValue`).
- `MLUtils.computeNumFeatures` and `VectorUDT.deserialize` both end at a
`SparseVector`, whose footprint is bounded by the actual `indices`/`values`
arrays. `SparseVector`'s own constructor already requires `size >= 0`,
`indices.length <= size`, and `maxIndex < size`. The dense allocation only
happens later in `toDense`/`toArray`, which this does not guard.
So the one site with a genuinely unbounded allocation is protected only by
an opt-in, off-by-default conf. It seems worth giving `AttributeGroup` an
always-on bound of its own (e.g. validate `numAttrs` against what
`attrMetadata` actually declares, or build the array lazily), and separately
reconsidering whether the per-row hook in `VectorUDT.deserialize` earns its
cost given that it does not prevent the densification allocation.
**6. A single global cap cannot distinguish an untrusted input dimension
from one Spark produced itself.**
`HashingTF` defaults to `numFeatures = 262144` and writes exactly that into
the column metadata via `new AttributeGroup($(outputCol), $(numFeatures))`.
Because the cap fires on the read-back path (`fromMetadata`,
`VectorUDT.deserialize`) rather than on ingestion, any cap below 262144 rejects
Spark's own intermediate results, not just the untrusted file. There is no way
to configure "bound what I read from the file, but not what my pipeline
produced".
**7. Could you spell out the threat model in the description?**
`SECURITY.md` defers to the project's security page for
in-scope/out-of-scope, and data read by a submitted application is normally
inside the submitter's trust boundary. A concrete scenario (third-party
libsvm/parquet handed to a shared cluster?) would help reviewers judge whether
a global cap is the right knob.
## Details
**8. The overflow in `computeNumFeatures` happens before the check.**
```scala
val numFeatures = rdd.map { ... }.reduce(math.max) + 1
MLMaxNumFeatures.check(numFeatures, ...)
```
An index of `Int.MaxValue` makes `numFeatures == Int.MinValue`, which passes
`check` (the guard is `count > max`). If the goal is to bound the inferred
dimension, compute in `Long` (`.toLong + 1`), check that, then narrow. Not
exploitable today since `Vectors.sparse` rejects a negative size downstream,
but it defeats the stated purpose of the check at that site.
**9. The two cache fields are published independently.**
`cachedValue` is written before `cachedEnv`, and both are separate
volatiles. If two `SparkEnv`s were ever live at once, a thread could publish
`cachedEnv = envA` after another published `cachedValue` for `envB`, pinning
the wrong value. Practically unreachable with one env per JVM, but a single
`@volatile private var cached: (SparkEnv, Int)` makes it correct for free.
**10. `MLMaxNumFeaturesSuite` creates and stops a `SparkSession` inside a
test body.**
It passes today, but it is order-dependent: `getOrCreate()` will attach to
any live `SparkContext` in the fork (the assert then fails rather than
corrupting, so it becomes a flake), and the `finally { spark.stop() }` would
stop a context the suite did not create. The idiomatic pattern for a static
conf is `SharedSparkSession` with `override protected def sparkConf` (see
`MetadataCacheSuite`), or adding a conf hook to `MLlibTestSparkContext` for
mllib.
**11. Minor:** the suite uses inline fully-qualified
`org.apache.spark.ml.linalg.VectorUDT` / `Vectors`; import aliases (as
`MLUtils.scala` already does with `MLVectorUDT`) would read better.
Style-wise the diff is clean -- no over-100-char lines, no non-ASCII,
license headers fine.
--
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]
