github-actions[bot] commented on code in PR #61196:
URL: https://github.com/apache/doris/pull/61196#discussion_r2915626187
##########
be/src/udf/python/python_env.h:
##########
@@ -148,14 +149,39 @@ class PythonVersionManager {
const std::string& python_venv_interpreter_paths);
Status get_version(const std::string& runtime_version, PythonVersion*
version) const {
+ if (!_env_scanner) {
+ return Status::Uninitialized(
+ "Set 'python_venv_interpreter_paths' in be.conf to enable
PythonUDF feature");
+ }
Review Comment:
[Medium] Inconsistent error message: This says
`python_venv_interpreter_paths` but the actual config gate is
`enable_python_udf_support`. When `enable_python_udf_support` is false,
`PythonVersionManager::init()` is never called, so `_env_scanner` is null
regardless of `python_venv_interpreter_paths`. The same inconsistency applies
to `get_envs()`, `env_type()`, and `to_string()` below.
Suggest using the same message as `env_infos_to_thrift()`: `"Set
'enable_python_udf_support = true' in be.conf to enable PythonUDF feature"`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowPythonPackagesCommand.java:
##########
@@ -128,7 +131,14 @@ public ShowResultSet doRun(ConnectContext ctx,
StmtExecutor executor) throws Exc
}
if (allBePackages.isEmpty()) {
- return new ShowResultSet(getMetaData(), Lists.newArrayList());
+ if (lastException != null) {
+ String msg = lastException.getMessage();
+ int newline = msg.indexOf('\n');
Review Comment:
[Low] `lastException.getMessage()` can return `null` for some exception
types (e.g., exceptions constructed without a message). If `msg` is null,
`msg.indexOf('\n')` will throw a `NullPointerException`. Consider guarding with:
```java
String msg = lastException.getMessage() != null ? lastException.getMessage()
: lastException.toString();
```
Same issue exists in `ShowPythonVersionsCommand.java`.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowPythonVersionsCommand.java:
##########
@@ -120,7 +123,14 @@ public ShowResultSet doRun(ConnectContext ctx,
StmtExecutor executor) throws Exc
}
List<List<String>> rows = Lists.newArrayList();
- if (commonVersions != null && !allBeEnvs.isEmpty()) {
+ if (allBeEnvs.isEmpty()) {
+ if (lastException != null) {
+ String msg = lastException.getMessage();
+ int newline = msg.indexOf('\n');
+ throw new AnalysisException("Failed to get python envs from
any backend: "
+ + (newline > 0 ? msg.substring(0, newline).trim() :
msg));
Review Comment:
[Low] When `allBeEnvs.isEmpty()` and `lastException == null` (i.e., all
backends are not alive), this falls through and returns an empty result set
silently. In contrast, `ShowPythonPackagesCommand` throws `"No alive backends
found to get python packages"` in the same scenario. The behavior should be
consistent — consider adding:
```java
} else {
throw new AnalysisException("No alive backends found to get python
envs");
}
```
--
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]
