[jira] [Comment Edited] (HIVE-22853) Beeline should use HS2 server defaults for fetchSize
[
https://issues.apache.org/jira/browse/HIVE-22853?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17049271#comment-17049271
]
David Mollitor edited comment on HIVE-22853 at 3/2/20 2:55 PM:
---
{code:java|title=BeeLine.java}
+ public int fetchSize = -1;
+ public void setFetchSize(int fetchRows) {
+if (fetchRows <= 0) { // if set to 0 or less, use server side default
+ this.fetchSize = Utils.getServerDefaultFetchSize();
+} else {
+ this.fetchSize = fetchRows;
+}
+ }
+
if (getOpts().timeout > -1) {
stmnt.setQueryTimeout(getOpts().timeout);
}
if (signalHandler != null) {
signalHandler.setStatement(stmnt);
}
+if (fetchSize > -1) {
+ stmnt.setFetchSize(fetchSize);
+}
{code}
# "-1" is an invalid value for fetch size, please make the default "0" so that
it lines up with the JDBC specs. The {{setFetchSize}} method makes sure the
value is always valid.
# Remove the blank line change to minimize the patch
# Do not check for a valid fetchSize, just set it
{{stmnt.setFetchSize(fetchSize);}}
was (Author: belugabehr):
{code:java|title=BeeLine.java}
+ public int fetchSize = -1;
+ public void setFetchSize(int fetchRows) {
+if (fetchRows <= 0) { // if set to 0 or less, use server side default
+ this.fetchSize = Utils.getServerDefaultFetchSize();
+} else {
+ this.fetchSize = fetchRows;
+}
+ }
+
if (getOpts().timeout > -1) {
stmnt.setQueryTimeout(getOpts().timeout);
}
if (signalHandler != null) {
signalHandler.setStatement(stmnt);
}
+if (fetchSize > -1) {
+ stmnt.setFetchSize(fetchSize);
+}
{code}
# "-1" is an invalid value for fetch size, please make the default "0" so that
it lines up with the JDBC specs. The {{setFetchSize}} method makes sure the
value is always valid.
# Remove the blank line change to minimize the patch
# Do not check for a valid fetchSize, just set it
{{stmnt.setFetchSize(fetchSize);}}}
> Beeline should use HS2 server defaults for fetchSize
>
>
> Key: HIVE-22853
> URL: https://issues.apache.org/jira/browse/HIVE-22853
> Project: Hive
> Issue Type: Bug
> Components: Beeline
>Affects Versions: 4.0.0
>Reporter: Naveen Gangam
>Assignee: Naveen Gangam
>Priority: Major
> Attachments: HIVE-22853.2.patch, HIVE-22853.3.patch,
> HIVE-22853.4.patch, HIVE-22853.5.patch, HIVE-22853.patch
>
>
> Currently beeline uses a hard coded default of 1000 rows for fetchSize. This
> default value is different from what the server has set. While the beeline
> user can reset the value via set command, its cumbersome to change the
> workloads.
> Rather it should default to the server-side value and set should be used to
> override within the session.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
[jira] [Comment Edited] (HIVE-22853) Beeline should use HS2 server defaults for fetchSize
[ https://issues.apache.org/jira/browse/HIVE-22853?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17047836#comment-17047836 ] Naveen Gangam edited comment on HIVE-22853 at 2/28/20 9:20 PM: --- New patch uploaded with some feedback from reviewers. 1) fetchsize = 0 should indicate to the client to use server side default. To meet this requirement, I had to make a server side code change as well. So the client sends its initial fetch size, set in JDBC URL or 0 otherwise as part of the OpenSession request. This value is set in session's config map. The server then responds with its configured fetch size using the same config key. (it felt unnecessary to add a new config property just for the response, or an undocumented key for the server side response). Instead I felt this would be cleaner. The client then save this server side value into a local variable, to be used whenever the client sets the fetch size to 0 or less than 0. 2) Setting a negative value for fetch size should throw an exception. The JDBC code does this currently. Throws a SQL exception. Uses the server side default on future statements. Beeline behavior is bit different. It does not indicate any issues with setting the negative value. This behavior made the most sense because we do not validate the values for other config properties set via beeline. I feel we should be consistent here. But it uses the server side default value until it has been reset. There was a lot of back and forth on whether we should ignore this set command entirely and only set it for positive values. The difference being, when you ignore the negative value set, we are using the last legally set value as opposed to the default. 3) Also switched to using a regex for more robustness with the set command with extra spaces in different part of the key/value. [~belugabehr] [~ychena] does this make sense? Could you please re-review? was (Author: ngangam): New patch uploaded with some feedback from reviewers. 1) fetchsize = 0 should indicate to the client to use server side default. To meet this requirement, I had to make a server side code change as well. So the client sends its initial fetch size, set in JDBC URL or 0 otherwise as part of the OpenSession request. This value is set in session's config map. The server then responds with its configured fetch size using the same config key. (it felt unnecessary to add a new config property just for the response, or an undocumented key for the server side response). Instead I felt this would be cleaner. The client then save this server side value into a local variable, to be used whenever the client sets the fetch size to 0 or less than 0. 2) Setting a negative value for fetch size should throw an exception. The JDBC code does this currently. Throws a SQL exception. Uses the server side default on future statements. Beeline behavior is bit different. It does not indicate any issues with setting the negative value. This behavior made the most sense because we do not validate the values for other config properties set via beeline. I feel we should be consistent here. But it uses the server side default value until it has been reset. There was a lot of back and forth on whether we should ignore this set command entirely and only set it for positive values. The difference being, when you ignore the negative value set, we are using the last legally set value as opposed to the default. [~belugabehr] [~ychena] does this make sense? Could you please re-review? > Beeline should use HS2 server defaults for fetchSize > > > Key: HIVE-22853 > URL: https://issues.apache.org/jira/browse/HIVE-22853 > Project: Hive > Issue Type: Bug > Components: Beeline >Affects Versions: 4.0.0 >Reporter: Naveen Gangam >Assignee: Naveen Gangam >Priority: Major > Attachments: HIVE-22853.2.patch, HIVE-22853.3.patch, > HIVE-22853.4.patch, HIVE-22853.patch > > > Currently beeline uses a hard coded default of 1000 rows for fetchSize. This > default value is different from what the server has set. While the beeline > user can reset the value via set command, its cumbersome to change the > workloads. > Rather it should default to the server-side value and set should be used to > override within the session. -- This message was sent by Atlassian Jira (v8.3.4#803005)
[jira] [Comment Edited] (HIVE-22853) Beeline should use HS2 server defaults for fetchSize
[
https://issues.apache.org/jira/browse/HIVE-22853?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17043763#comment-17043763
]
David Mollitor edited comment on HIVE-22853 at 2/24/20 6:56 PM:
OK. I just you that there is value here, I'm not sure I see it.
If you want to do this, I don't think the current approach is correct.
[https://docs.oracle.com/javase/8/docs/api/java/sql/Statement.html#setFetchSize-int-]
{quote}
If the value specified is zero, then the hint is ignored. The default value is
zero.
SQLException - the condition rows >= 0 is not satisfied.
{quote}
So, I think you should adhere more closely to this spec to avoid ambiguity and
you don't need to check for a valid value. Just always set the fetch size on
the statement. Also think about what is the behavior if the user attempts to
set something less than zero? Right now the patch ignores the value, I'm not
sure that's the best approach. Better to validate the user supplied value and
log an error if the value is invalid otherwise user will not know that their
change didn't take affect.
Also, the proposed code is too brittle. I would like to see this actually
parse the statement and then look for the value instead of just this simple,
brittle string match. Would HiveServer2 accept the following?
{code:none}
set hive.server2.thrift.resultset.default.fetch.size=1;
set hive.server2.thrift.resultset.default.fetch.size =1
set hive.server2.thrift.resultset.default.fetch.size = 1;
{code}
Would HS2 accept these? Would your solution?
was (Author: belugabehr):
OK. I just you that there is value here, I'm not sure I see it.
If you want to do this, I don't think the current approach is correct.
[https://docs.oracle.com/javase/8/docs/api/java/sql/Statement.html#setFetchSize-int-]
{quote}
If the value specified is zero, then the hint is ignored. The default value is
zero.
SQLException - the condition rows >= 0 is not satisfied.
{quote}
So, I think you should adhere more closely to this spec to avoid ambiguity and
you don't need to check for a valid value. Just always set the fetch size on
the statement. Also think about what is the behavior if the user attempts to
set something less than zero? Right now the patch ignores the value, I'm not
sure that's the best approach. Better to validate the user supplied value and
log an error if the value is invalid otherwise user will not know that their
change didn't take affect.
Also, the proposed code is too brittle. I would like to see this actually
parse the statement and then look for the value instead of just this simple,
brittle string match. Would HiveServer2 accept the following?
{code:none}
set hive.server2.thrift.resultset.default.fetch.size=1;
set hive.server2.thrift.resultset.default.fetch.size=1;
{code}
> Beeline should use HS2 server defaults for fetchSize
>
>
> Key: HIVE-22853
> URL: https://issues.apache.org/jira/browse/HIVE-22853
> Project: Hive
> Issue Type: Bug
> Components: Beeline
>Affects Versions: 4.0.0
>Reporter: Naveen Gangam
>Assignee: Naveen Gangam
>Priority: Major
> Attachments: HIVE-22853.2.patch, HIVE-22853.3.patch, HIVE-22853.patch
>
>
> Currently beeline uses a hard coded default of 1000 rows for fetchSize. This
> default value is different from what the server has set. While the beeline
> user can reset the value via set command, its cumbersome to change the
> workloads.
> Rather it should default to the server-side value and set should be used to
> override within the session.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
