cloud-fan commented on PR #53570:
URL: https://github.com/apache/spark/pull/53570#issuecomment-3982113106
*This review was generated by Cursor with Claude Opus 4.6.*
Additional implementation review comments beyond what's already been raised:
### 1. `lookupFunctionInfo` — table functions with `kind == Temp` skip view
context filtering (bug)
For `kind == Temp`, the scalar branch applies view context filtering via
`lookupTempFuncWithViewContext`, but the table function branch does a plain
`registry.functionExists / registry.lookupFunction` with no view filtering:
```scala
case SessionCatalog.Temp =>
synchronized {
if (tableFunction) {
// No view context filtering!
if (registry.functionExists(identifier))
registry.lookupFunction(identifier) else None
} else {
lookupTempFuncWithViewContext(
name, _ => false, _ => registry.lookupFunction(identifier))
}
}
```
This means a temp table function could be visible inside a view even when it
shouldn't be. Both branches should apply `handleViewContext`.
### 2. `registerUserDefinedFunction` — `isTableFunction` flag is redundant
The method already receives `registry: FunctionRegistryBase[T]` which is
either `functionRegistry` or `tableFunctionRegistry`. The "other" registry can
be derived:
```scala
val otherRegistry = if (registry eq functionRegistry) tableFunctionRegistry
else functionRegistry
```
No need for the extra `isTableFunction` parameter.
### 3. `resolveQualifiedTableFunction` — `case _: Exception => // ignore` is
dangerous
In the inner try-catch that checks whether a scalar function exists (to
throw `NOT_A_TABLE_FUNCTION`), all exceptions are silently swallowed:
```scala
} catch {
case _: Exception => // ignore
}
```
This could mask real errors (permission failures, catalog connectivity,
etc.). It should catch only the specific exceptions that indicate "function not
found" — `NoSuchFunctionException`, `NoSuchNamespaceException`,
`CatalogNotFoundException`, and the `FORBIDDEN_OPERATION` `AnalysisException`
(matching the outer catch).
### 4. `FunctionType` sealed trait is over-engineered for its single use
`FunctionType` (with 5 case objects) is only used by `lookupFunctionType`,
which is only called from `LookupFunctions` in `Analyzer.scala`. The
`Builtin`/`Temporary` distinction is never actually used — both hit the `throw
SparkException.internalError(...)` branch. Consider simplifying or inlining the
logic.
### 5. `resolveBuiltinOrTempTableFunctionRespectingPathOrder` — appears to
be dead code
This public method returns `Option[Either[LogicalPlan, Unit]]` but doesn't
appear to be called anywhere in the diff. If it is needed, `Right(())` to
signal "scalar found but no table function" is not self-documenting and should
use a clearer type. If it's not called, it should be removed.
### 6. `createOrReplaceTempFunction` — the `source == "internal"` check is
dead code
The PR adds a `source == "internal"` branch to avoid qualifying internal
functions with `SESSION_NAMESPACE`. But `FunctionRegistry.internal` is a
completely separate `SimpleFunctionRegistry` instance — internal functions are
registered via `registerInternalExpression` directly into that registry, never
through `createOrReplaceTempFunction`. No caller ever passes `"internal"` as
the source. The conditional should be removed and the method should always
qualify with `SESSION_NAMESPACE`.
### 7. Duplicated scaladoc on `lookupFunctionWithShadowing`
There are two consecutive scaladoc blocks before
`lookupFunctionWithShadowing` — the first is an orphaned leftover that should
be removed.
### 8. Indentation-only changes pollute the diff
Several places in `SessionCatalog.scala` have whitespace-only changes that
shift indentation without any logical change (e.g. `fetchCatalogFunction`, the
persistent branch of `registerUserDefinedFunction`, `registerFunction` call
sites). These should be reverted to keep `git blame` clean.
---
_This comment was generated with [GitHub MCP](http://go/mcps)._
--
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]
