spark-test commented on code in PR #59980:
URL: https://github.com/apache/spark/pull/59980#discussion_r4466146674
##########
python/pyspark/sql/transpile.py:
##########
@@ -928,6 +928,13 @@ def _get_transpilers(session: "SparkSession") ->
List[AbstractTranspiler]:
if not configured_transpilers:
return []
transpiler_names = configured_transpilers.split(",")
+ unknown_names = [name for name in transpiler_names if name not in
AbstractTranspiler.varieties]
Review Comment:
**Non-blocking (P2):** `unknown_names` is built from the raw `split(",")`
entries, so an empty or whitespace-only entry (from a trailing or doubled
comma, or `"catalyst, "`) is reported as an unknown transpiler, although it
names nothing and the returned list is the same as for `"catalyst"`. With
`pyTranspilers="catalyst,"`, every UDF definition that reaches transpilation
(`transpilePyUDFs=true`, ANSI on, classic session) now warns `Ignoring unknown
transpilers [''] ...`. Under warnings-as-errors (`-W error`, or pytest's
`filterwarnings = error`), defining a UDF that lowered on the base branch now
fails with `Exception transpiling UDF ...` instead. Spark's other
comma-separated confs treat such entries as formatting
(`SparkStringUtils.stringToSeq` drops them).
Skipping blank entries in this check, e.g. `if name.strip() and name not in
AbstractTranspiler.varieties`, avoids this. A padded name such as `" catalyst"`
should still be reported, because selection does not match it either.
**Verification:**
- **Behavior:** For transpiler-name values that differ only by trailing,
doubled, or whitespace-only entries, the selected transpilers are identical and
no unknown-transpiler warning is recorded.
- **Regression:** With transpilation and ANSI enabled and warnings escalated
to errors, defining a lowerable UDF succeeds and produces lowered options for
each transpiler-name value whose only unregistered entries are empty or
whitespace-only, covering trailing ('catalyst,'), doubled
('catalyst,,catalyst') and whitespace-only ('catalyst, ') entries.
- **Compatibility:** Values with a non-blank unregistered entry keep their
current transpiler selection and still produce exactly one warning naming
exactly those entries.
##########
python/pyspark/sql/transpile.py:
##########
@@ -928,6 +928,13 @@ def _get_transpilers(session: "SparkSession") ->
List[AbstractTranspiler]:
if not configured_transpilers:
return []
transpiler_names = configured_transpilers.split(",")
+ unknown_names = [name for name in transpiler_names if name not in
AbstractTranspiler.varieties]
+ if unknown_names:
+ warnings.warn(
Review Comment:
**Non-blocking (P2):** This warning is raised inside `_transpile_func`'s
catch-all: `_get_transpilers` is called at line 1385 within the `try` whose
`except Exception` (line 1406) returns no options. Once a warnings filter turns
the UserWarning into an exception, the warn raises before the registered
transpilers are returned, so none of them is tried, although the message says
only the unknown entries are ignored.
For example, take `pyTranspilers="catalyst,catalist"` with
`simplefilter("error")` plus an ignore for `Unable to transpile UDF`, which
warnings-as-errors users already need in order to define UDFs that cannot be
lowered. A UDF that the base branch lowered with `catalyst` now gets no lowered
options and no visible warning. An error filter aimed only at this message does
the same, and the requested error never surfaces: the user sees only a
non-fatal `Unable to transpile UDF ...` warning. The UDF then runs as a plain
Python UDF (BatchEvalPythonExec, no codegen or filter pushdown), and a
non-inner join condition in which it references both sides fails to plan.
Results stay correct, and only UDF definitions that reach transpilation
(`transpilePyUDFs=true`, ANSI on, classic session) are affected. The module
already avoids this hazard for the source parse: `_syntax_warnings_suppressed`
exists because otherwise "under warnings-as-errors the parse raises and
lowering silently turns off".
**Recommended change:** Stop raising the unknown-transpiler warning inside
transpiler selection. Have selection return the unregistered entries alongside
the transpilers it keeps, and pass them out of the transpilation attempt with
its existing results. The UDF constructor then issues the single UserWarning
after the attempt and outside its fallback handling. Update the PySpark UDF
transpile unit tests to check the warning at UDF definition, and the outcome
under an escalating warnings filter.
**Why this works:** Selection of the registered transpilers, and lowering
with them, now completes before any warning is raised, and neither fallback
handler can catch the warning. Without escalation, the UDF keeps its lowered
options and the user sees the warning. When a filter escalates the warning, the
exception carrying the unknown-name message propagates from UDF definition, so
the diagnostic can no longer silently switch lowering off.
**Scope:** Move the reporting of unregistered transpiler names from inside
the transpiler's catch-all to the UDF constructor, after the transpilation
attempt, and test it at UDF definition.
**Compatibility:** Without escalation, the transpilers selected for every
pyTranspilers value, and the resulting lowering, stay as on the base branch.
The transpilation attempt keeps its rule that an inability to transpile never
breaks a working UDF.
**Risks:** Under a filter that escalates this warning, defining a UDF while
pyTranspilers contains an unregistered entry raises the unknown-name
UserWarning directly. On the base branch such a value produced no warning at
all, and the current patch instead raises a nested fallback warning or runs the
UDF without lowering. That is the explicit outcome the filter asks for, and
ignoring the message restores silent operation.
**Constraints:** The transpilation attempt must still never raise because a
function cannot be transpiled, as its existing fallback contract requires. Only
the constructor-level warning may propagate, and only when a filter escalates
it. Do not add a conf read to UDF definition; carry the unregistered entries
out of the existing transpilation call.
**Exclusions:**
- Which configured entries count as unregistered, for example empty entries
left by a stray comma, is handled by the separate fix for blank entries.
**Success:** With transpilation enabled, ANSI on, and pyTranspilers set to a
registered plus an unregistered name such as 'catalyst,catalist', defining a
lowerable UDF under default filters keeps its lowered options and emits exactly
one UserWarning naming the unregistered entry. Under a warnings filter that
escalates that UserWarning, including one that ignores the 'Unable to transpile
UDF' fallback warning, defining the UDF raises the unknown-name warning and
never yields a silently unlowered UDF. A value whose entries are all
registered, such as the default 'catalyst', emits no warning and lowers exactly
as on the base branch under every warnings filter.
**Verification:**
- **Regression:** With a registered plus an unregistered name configured,
defining a lowerable UDF raises the unknown-name UserWarning, instead of
producing a UDF with no lowered options, under each warnings filter that
escalates that warning while leaving the 'Unable to transpile UDF' fallback
warning non-fatal or ignored, covering at least all warnings escalated with
that fallback message ignored, and an error filter for the unknown-name message
alone.
- **Behavior:** Under default warnings filters, defining a lowerable UDF
with a mixed transpiler-name value keeps non-empty lowered options and records
exactly one warning naming only the unregistered entries.
- **Compatibility:** With the default transpiler-name value, defining a UDF
emits no new warning and produces the same lowered options under default and
error warnings filters.
##########
python/pyspark/sql/tests/test_udf_transpile_unit.py:
##########
@@ -179,6 +180,27 @@ def plus_four(x):
"when both transpilePyUDFs and ANSI mode are enabled",
)
+ def test_get_transpilers_warns_on_unknown_names(self):
+ import warnings
+
+ for names, expected, unknown in [
+ ("catalyst", [CatalystTranspiler], None),
+ ("catalyst,catalist", [CatalystTranspiler], "catalist"),
+ ("catalist", [], "catalist"),
+ ]:
+ with self.subTest(names=names):
+ with
self.sql_conf({"spark.sql.experimental.optimizer.pyTranspilers": names}):
+ with warnings.catch_warnings(record=True) as caught:
+ warnings.simplefilter("always")
+ transpilers = _get_transpilers(self.spark)
+ self.assertEqual([type(t) for t in transpilers], expected)
+ messages = [str(w.message) for w in caught if "pyTranspilers"
in str(w.message)]
+ if unknown is None:
+ self.assertEqual(messages, [])
+ else:
+ self.assertEqual(len(messages), 1)
+ self.assertIn(repr(unknown), messages[0])
Review Comment:
**Nit (P3):** These assertions only check that one message mentions
`pyTranspilers` and contains `repr(unknown)`. A regression that also reports
`catalyst` as ignored, such as `Ignoring unknown transpilers ['catalyst',
'catalist'] ...; registered transpilers are ['catalist']`, passes every
subtest. Asserting the full expected message, or the exact ignored and
registered lists, for each case would pin the content this PR adds.
**Verification:**
- **Regression:** A variant of the warning that keeps the same transpiler
selection but lists 'catalyst' among the ignored entries, or misstates the
registered transpilers, makes the unregistered-name test fail.
--
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]
