HIVE-27490: HPL/SQL says it support default value for parameters but not considering them when no value is passed #5084
Conversation
…not considering them when no value is passed
asf-ci-hive
added
tests failed
tests pending
tests passed
and removed
tests pending
tests failed
tests passed
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests passed
and removed
tests passed
tests pending
labels
hplsql/src/main/java/org/apache/hive/hplsql/functions/InMemoryFunctionRegistry.java
Outdated
Show resolved
Hide resolved
| actualCnt = actualCnt + defaultParamNamesVsIndexes.size(); | ||
| if (actualCnt < formalCnt) { | ||
| throw new ArityException(ruleContext, procName, formalCnt, passedParamCnt); | ||
| } |
There was a problem hiding this comment.
actualCnt can be higher then formalCnt. Let's say we have a function with 2 formal parameters both has default values. When calling the function we pass only one actual parameter. The result is actualCnt = 1 + 2 = 3 which is incorrect.
What is the purpose of this check here? if (actualCnt < formalCnt)
The check in the end of the method should catch such cases too
if ((passedParamCnt + defaultParamNamesVsIndexes.size()) < formalCnt) {
There was a problem hiding this comment.
actualCnt can be higher then formalCnt. Let's say we have a function with 2 formal parameters both has default values. When calling the function we pass only one actual parameter. The result is actualCnt = 1 + 2 = 3 which is incorrect.
Here actualCnt contains the actual parameters passed + all default parameters of the procedure. Which can be greater than formalCnt as user can pass values for some default params along with non-default params. For the same example a function with 2 formal parameters both has default values. When calling the function we pass only one actual parameter then actualCnt = 1 + 2 = 3 is correct why because user passed one parameter and for other parameter we take default value and execute the procedure. Thats why here we are checking only actualCnt < formalCnt.
What is the purpose of this check here? if (actualCnt < formalCnt)
As explained above if user passes lesser parameters than the required parameters then we should throw exception. For example if a function has 5 total params, in that lets say 2 default params, so here user has to pass minimum 3 param values but if user passes only 2 then 2+2=4 which is lesser than formal count so need to throw exception.
The check in the end of the method should catch such cases too
if ((passedParamCnt + defaultParamNamesVsIndexes.size()) < formalCnt) {
This check is done after processing the values. This is useful if user passes param values in named parameter binding. For example if a function takes 3 params in that 1 param is default one. Here user passes the values for one non-default param and default param using named parameter binding then we should throw exception as one non-default param value is not passed.
There was a problem hiding this comment.
Isn't this last check enough for all scenarios?
There was a problem hiding this comment.
Using last check now.
hplsql/src/main/java/org/apache/hive/hplsql/functions/InMemoryFunctionRegistry.java
Outdated
Show resolved
Hide resolved
| if (formalCnt != actualCnt) { | ||
| throw new ArityException(actual.getParent(), procName, formalCnt, actualCnt); | ||
| int actualCnt = (actualValues == null) ? 0 : actualValues.size(); | ||
| int passedParamCnt = actualCnt; |
There was a problem hiding this comment.
What is the difference between passedParamCnt and actualCnt? Can we remove one of them?
There was a problem hiding this comment.
passedParamCnt is the actual params count which user passed while calling the function. actualCnt is actual params passed + default param count(updating actualCnt value later in the flow (actualCnt = actualCnt + defaultParamNamesVsIndexes.size();)
There was a problem hiding this comment.
I think the sum of number of params passed by the user and the number of parameters with default values doesn't makes sense because there can be overlaps.
Please remove one of the variables.
| private static void populateDefaultParamDetails(List<HplsqlParser.Create_routine_param_itemContext> routineParamItem, int formalCnt, | ||
| Map<String, Integer> defaultParamNamesVsIndexes) { | ||
| for (int i = 0; i < formalCnt; i++) { |
There was a problem hiding this comment.
how about
for (int i = 0; i < routineParamItem.size(); i++)
and formalCnt can be removed from param list.
asf-ci-hive
added
tests pending
tests failed
and removed
tests passed
tests pending
labels
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
|
HIVE-27490: HPL/SQL says it support default value for parameters but not considering them when no value is passed
What changes were proposed in this pull request?
The changes support considering default values specified for that parameter when value is not passed for the param of a HPLSQL procedure.
Why are the changes needed?
The change is required to support considering default values specified for that parameter when value is not passed for the param of a HPLSQL procedure.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
Tests are added.
mvn test -Dtest=TestHplSqlViaBeeLine -pl itests/hive-unit -Pitests