luoyuxia commented on code in PR #22294:
URL: https://github.com/apache/flink/pull/22294#discussion_r1169457878
##########
flink-table/flink-sql-gateway/src/main/java/org/apache/flink/table/gateway/service/operation/OperationExecutor.java:
##########
@@ -182,23 +181,17 @@ public ResultFetcher configureSession(OperationHandle
handle, String statement)
public ResultFetcher executeStatement(OperationHandle handle, String
statement) {
// Instantiate the TableEnvironment lazily
- // TODO: remove the usage of the context classloader until {@link
Review Comment:
Althogh the comment said we need make `HiveParserUtils#getFunctionInfo` use
ResourceManager explicitly.
But after do some investigation, I found it's not necessary.
Currently, there's two callers to call method
`HiveParserUtils#getFunctionInfo`
1:
```java
FunctionInfo fi = HiveParserUtils.getFunctionInfo(funcText);
if (fi == null) {
desc = convertSqlOperator(funcText, children, ctx);
}
....
```
The `fi` will be null after we remove the classloader wrapper as it can't
load the function using `Thread.currentThread().getContextClassLoader()`. But
it's fine since it will then try to find the function from the function
catalog. Such case have been convered in flink-sql-client by `set.q`.
2: `getFuncExprNodeDescWithUdfData`
There're two places call method `getFuncExprNodeDescWithUdfData`.
a: convertGenericFunc(ExprNodeGenericFuncDesc func), but it will only be
called when it's hive build-in function, so it can be found in
`Thread.currentThread().getContextClassLoader()` as use must put
flink-connector-hive jar which include hive built-in functions to
FLINK_HOME/lib to use Hive Dialect.
b:
```java
// type conversion for LHS
if (TypeInfoUtils.isConversionRequiredForComparison(lhsType,
commonType)) {
lhsDesc =
HiveASTParseUtils.createConversionCast(
lhsDesc, (PrimitiveTypeInfo) commonType);
}
```
Also, ` HiveParserUtils.getFunctionInfo(funcText)` will only be called to
when it's build-in function which can then be loaded.
AFAIC, it's not easy to make `HiveParserUtils#getFunctionInfo` use
ResourceManager explicitly which will involves much modification. I think it's
fine to remove the usage of the context classloader in here.
cc @fsk119
--
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: issues-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
