Copilot commented on code in PR #6412:
URL: https://github.com/apache/hive/pull/6412#discussion_r3557478705
##########
jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java:
##########
@@ -398,20 +398,73 @@ private TGetOperationStatusResp waitForResultSetStatus()
throws SQLException {
return statusResp;
}
+ /**
+ * Returns the timeout message for a {@code TIMEDOUT_STATE} response. The
server is authoritative:
+ * when the SQL state is {@code HYT00} ("timeout expired") it has set a
precise message that
+ * already reflects the effective {@code hive.query.timeout.seconds}, so it
is used verbatim.
+ * Otherwise (e.g. an older server) falls back to the per-statement {@link
#setQueryTimeout(int)}.
+ */
+ private String sqlTimeoutMessageForTimedOutState(String serverMessage,
String sqlState) {
+ if ("HYT00".equals(sqlState) && StringUtils.isNotBlank(serverMessage)) {
+ return serverMessage;
+ }
+ if (queryTimeout > 0) {
+ return "Query timed out after " + queryTimeout + " seconds";
+ }
+ return "Query timed out";
+ }
Review Comment:
The PR description mentions ignoring/overriding clearly wrong server timeout
text like “after 0 seconds”, but the current logic returns any non-blank
serverMessage verbatim when sqlState is HYT00. If an older/mixed-version server
still sends “after 0 seconds”, the client will continue to surface that
incorrect message. Consider accepting any non-blank serverMessage only when it
doesn’t contain the known-bad “after 0 seconds” substring, and otherwise
falling back to the client-generated message.
##########
jdbc/src/java/org/apache/hive/jdbc/HiveStatement.java:
##########
@@ -398,20 +398,73 @@ private TGetOperationStatusResp waitForResultSetStatus()
throws SQLException {
return statusResp;
}
+ /**
+ * Returns the timeout message for a {@code TIMEDOUT_STATE} response. The
server is authoritative:
+ * when the SQL state is {@code HYT00} ("timeout expired") it has set a
precise message that
+ * already reflects the effective {@code hive.query.timeout.seconds}, so it
is used verbatim.
+ * Otherwise (e.g. an older server) falls back to the per-statement {@link
#setQueryTimeout(int)}.
+ */
Review Comment:
The Javadoc claims the server message “already reflects the effective
hive.query.timeout.seconds”, but on the server side the timeout used is the
effective *operation* timeout (min of session hive.query.timeout.seconds and
any per-statement query timeout). This wording is misleading for cases where
Statement#setQueryTimeout(int) is smaller than the session setting.
--
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]
