HIVE-29555: [HPLSQL]Errorcode handling is not proper in HPLSQL.

[mdayakar]

HIVE-29555: [HPLSQL]Errorcode handling is not proper in HPLSQL.

What changes were proposed in this pull request?

When SQL statements are executed using EXECUTE/EXECUTE IMMEDIATE, in positive scenario the ERRORCODE is not set as SUCCESS. The default value for ERRORCODE is SUCCESS so even without setting the errorcode to success in positive scenario is fine but if the ERRORCODE is set as ERROR as a part of previous statement execution then another statement is executed successfully but still ERRORCODE is already set as ERROR which is not correct. Now in positive flow also we are setting SUCCESS for the ERRORCODE.

Why are the changes needed?

To fix the error reported in HIVE-29555.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added testcase to test the scenario.
mvn test -Dtest=TestHplSqlViaBeeLine#testERRORCODEForExecuteStatements -pl itests/hive-unit -Pitests

[thomasrebele]

I've checked some other methods in the classes Stmt and Exec for potential bugs

• Stmt#fetch when entering the if (ctx.bulk_collect_clause() != null), no status is set
• Stmt#return_ calls Exec#signal(Signal.Type), which always setSqlCode(SqlCodes.ERROR)
• Stmt#allocateCursor does not set the status
• There are a few setSqlCode(SqlCodes.NO_DATA_FOUND), even though there's a dedicated method Exec#setSqlNoData; moreover, 2 out of 3 of these calls are directly followed by a exec.signal(Signal.Type.NOTFOUND), which overrides the SQL code to SqlCodes.ERROR

Some places seem to not set the SQL state:

• Stmt#use uses exec.setSqlCode(SqlCodes.SUCCESS) instead of exec.setSqlSuccess()
• Expression#execCursorAttribute, if (cursorVar != null)
• Exec#execHost

I think these cases should be handled as well, either in this ticket, or by a follow-up ticket.

[mdayakar]

Thanks for the detailed review @thomasrebele

Stmt#fetch when entering the if (ctx.bulk_collect_clause() != null), no status is set -- Fixed, setting Sql SUCCESS status.
Stmt#return_ calls Exec#signal(Signal.Type), which always setSqlCode(SqlCodes.ERROR) -- Fixed, now calling Exec#signal(Signal.Type, null, null) which will set only signal state.
Stmt#allocateCursor does not set the status -- Fixed, setting Sql SUCCESS status.
There are a few setSqlCode(SqlCodes.NO_DATA_FOUND), even though there's a dedicated method Exec#setSqlNoData; moreover, 2 out of 3 of these calls are directly followed by a exec.signal(Signal.Type.NOTFOUND), which overrides the SQL code to SqlCodes.ERROR -- Fixed, now calling Exec#setSqlNoData; in all places where setSqlCode(SqlCodes.NO_DATA_FOUND) API is called.

Some places seem to not set the SQL state:

Stmt#use uses exec.setSqlCode(SqlCodes.SUCCESS) instead of exec.setSqlSuccess() -- Fixed, Changed exec.setSqlCode(SqlCodes.SUCCESS) to exec.setSqlSuccess().
Expression#execCursorAttribute, if (cursorVar != null) -- Here cursor attribute is set based on %ISOPEN, %FOUND and %NOTFOUND, the return value will be either true/false/null so no need of setting ERRORCODE here.
Exec#execHost -- Here already HOSTCODE is getting set.

[sonarqubecloud]

Quality Gate passed

Issues
5 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[thomasrebele]

LGTM, with a minor comment.

[thomasrebele]

Do we need to call setSqlSuccess() again? As far as I understand the code, the SQL code or state are now overwritten by the previous commands.

[mdayakar]

It may be redundant here but it won't override anything as the same statement we are calling above in case of success. Anyway I will remove this statement as a part of some other PR as currently I could see some random failures everytime, now its green :).

[thomasrebele]

Yes, no need to change it if the CI is green :)