You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2021/01/15 17:47:48 UTC
[GitHub] [hive] jfsii opened a new pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
jfsii opened a new pull request #1876:
URL: https://github.com/apache/hive/pull/1876
Change-Id: Ic6ddbf9ffb14c293caadf1f8ec1e7ec0f9291a31
### What changes were proposed in this pull request?
UDF configure does not get called if fetch task conversion happens.
This attempts to resolve this by calling configure with a dummy MapredContext.
It is a bit ugly in that MapredContext seems to be legacy baggage with various members that do not make sense for a variety of engines and code.
### Why are the changes needed? Some custom UDFs are designed with the expectation that configure is called in all instances. Users sometimes use it to inspect configuration values or to set new values to propagate to the evaluate method or to do one time setup.
### Does this PR introduce _any_ user-facing change? Yes - it will now call configure in cases where configure was not called before.
### How was this patch tested? Added udf_configure.q and manual testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jfsii commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-764002826
Rebased - also ran tests locally and it seemed to pass. Going to hope it was an infra issue prior.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk merged pull request #1876: HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
kgyrtkirk merged pull request #1876:
URL: https://github.com/apache/hive/pull/1876
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-767082017
@jfsii , are the failures related? Could you trigger testing again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1876:
URL: https://github.com/apache/hive/pull/1876#discussion_r566284005
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/ExprNodeGenericFuncEvaluator.java
##########
@@ -140,8 +141,23 @@ public ObjectInspector initialize(ObjectInspector rowInspector) throws HiveExcep
childrenOIs[i] = children[i].initialize(rowInspector);
}
MapredContext context = MapredContext.get();
+ // It is possible that there is no context at this point. For example a context is not created
+ // when "hive.fetch.task.conversion" occurs.
if (context != null) {
context.setup(genericUDF);
+ } else {
+ // It is a bit unfortunate that currently the UDF configuration signature expects a
+ // MapredContext (even if execution is tez or another engine) - this causes an
+ // impedence mismatch. For example: MapredContext has Reporter objects that may or
+ // may not make sense for the current engine.
+ //
+ // We attempt to create a dummyContext that has at least access to the currently set
+ // configuration. The other unfortunate issue is that some paths set the configuration
+ // when creating ExprNodeGenericFuncEvaluator while others do not, so we fallback to a
+ // "default" new HiveConf object which might be missing configuration that changed during
+ // runtime.
+ MapredContext dummyContext = MapredContext.createDummy(getConf() != null ? getConf() : new HiveConf());
Review comment:
this is strange - during fetch task conversion we should have access to the sessionstate; and when we are running it on the cluster mapredcontext/tezcontext should be available.
There is one thing which might have came up in the failures: when we run HS2/clidriver we *always* have a session state when we run queries; because of this nature I think it makes sense to rely on this mechanism instead of working around stuff here and there - however some test(s) may have "forgot" to start the session (`SessionState.start`) - which could cause an NPE ; I think this might be happening in `TestVectorizationContext`
but...there is also some stuff inside the metastore which can't have a session - so you might need to handle the null case as well...
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #1876:
URL: https://github.com/apache/hive/pull/1876#discussion_r565476709
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/ExprNodeGenericFuncEvaluator.java
##########
@@ -140,8 +141,23 @@ public ObjectInspector initialize(ObjectInspector rowInspector) throws HiveExcep
childrenOIs[i] = children[i].initialize(rowInspector);
}
MapredContext context = MapredContext.get();
+ // It is possible that there is no context at this point. For example a context is not created
+ // when "hive.fetch.task.conversion" occurs.
if (context != null) {
context.setup(genericUDF);
+ } else {
+ // It is a bit unfortunate that currently the UDF configuration signature expects a
+ // MapredContext (even if execution is tez or another engine) - this causes an
+ // impedence mismatch. For example: MapredContext has Reporter objects that may or
+ // may not make sense for the current engine.
+ //
+ // We attempt to create a dummyContext that has at least access to the currently set
+ // configuration. The other unfortunate issue is that some paths set the configuration
+ // when creating ExprNodeGenericFuncEvaluator while others do not, so we fallback to a
+ // "default" new HiveConf object which might be missing configuration that changed during
+ // runtime.
+ MapredContext dummyContext = MapredContext.createDummy(getConf() != null ? getConf() : new HiveConf());
Review comment:
you might have access to the session conf via `SessionsState.get().getConf()` - which should be much closer to the actual configuration than `new HiveConf()`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on a change in pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jfsii commented on a change in pull request #1876:
URL: https://github.com/apache/hive/pull/1876#discussion_r565532925
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/exec/ExprNodeGenericFuncEvaluator.java
##########
@@ -140,8 +141,23 @@ public ObjectInspector initialize(ObjectInspector rowInspector) throws HiveExcep
childrenOIs[i] = children[i].initialize(rowInspector);
}
MapredContext context = MapredContext.get();
+ // It is possible that there is no context at this point. For example a context is not created
+ // when "hive.fetch.task.conversion" occurs.
if (context != null) {
context.setup(genericUDF);
+ } else {
+ // It is a bit unfortunate that currently the UDF configuration signature expects a
+ // MapredContext (even if execution is tez or another engine) - this causes an
+ // impedence mismatch. For example: MapredContext has Reporter objects that may or
+ // may not make sense for the current engine.
+ //
+ // We attempt to create a dummyContext that has at least access to the currently set
+ // configuration. The other unfortunate issue is that some paths set the configuration
+ // when creating ExprNodeGenericFuncEvaluator while others do not, so we fallback to a
+ // "default" new HiveConf object which might be missing configuration that changed during
+ // runtime.
+ MapredContext dummyContext = MapredContext.createDummy(getConf() != null ? getConf() : new HiveConf());
Review comment:
Thanks, I'll give that a try since a fully formed one is better (also I think new HiveConf is slightly expensive looking at all the stuff the construction takes).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #1876: HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jfsii commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-777657504
finally got green on this one
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jfsii commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-776006033
So I think I am down to the flaky test(s) - no change between this commit and last but got different failures. This time I got one: TestZookeeperLockManager
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jfsii commented on pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jfsii commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-764002826
Rebased - also ran tests locally and it seemed to pass. Going to hope it was an infra issue prior.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] jcamachor commented on pull request #1876: [WIP] HIVE-24645: Call configure for UDFs after fetch task conversion
Posted by GitBox <gi...@apache.org>.
jcamachor commented on pull request #1876:
URL: https://github.com/apache/hive/pull/1876#issuecomment-767082017
@jfsii , are the failures related? Could you trigger testing again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org