[CALCITE-2322] Add fetch size support to connection url and JDBC state… #49
Conversation
There was a problem hiding this comment.
I looked at a couple of jdbc driver, and looks like connection property is named defaultFetchSize or equivalent, because this is the value to be used when Statement/ResultSet fetchSize is set to 0.
Currently, the code doesn't check that the value set in fetchSize is 0 or greater. Also if value is 0, I don't think the value is then replaced by the default (same for AvaticaResultSet). The reason is probably because it's not possible to change the fetch size for the current current cursor, that said it would be nice to clean up a bit AvaticaStatement and AvaticaResult (and ConnectionConfig) to validate (default) fetch size value, and in case 0 is used to replace it by the default value on the fly (and not just at construction time)
| @@ -108,6 +109,7 @@ protected AvaticaStatement(AvaticaConnection connection, | |||
| this.resultSetType = resultSetType; | |||
| this.resultSetConcurrency = resultSetConcurrency; | |||
| this.resultSetHoldability = resultSetHoldability; | |||
| this.fetchSize = connection.config().fetchSize(); // Default to connection config fetch size. | |||
There was a problem hiding this comment.
let's remove the default value set for the field declaration
There was a problem hiding this comment.
I'm not clear on what you are suggesting here. Are you suggesting this be defaulted to 0 and have the real value resolved in getFetchSize()? I went with the approach I did because a setting made via the connection url is accessed through the connection object. The connection reference isn't stored within AvaticaStatement so the connection value needs to be propagated down somehow. You are correct that if the value is explicitly set to 0 either via url param or via Statement.setFetchSize or ResultSet.setFetchSize I have no idea what will happen. All I can say is that I didn't change that behavior with this change. It will do whatever is would have done before.
There was a problem hiding this comment.
the value is both set in the field declaration and in the constructor, just suggesting to remove the default value in the field initialization...
| HostnameVerification.class, false), | ||
|
|
||
| /** Fetch size limit, default is 100 rows. */ | ||
| FETCH_SIZE("fetch_size", Type.NUMBER, AvaticaStatement.DEFAULT_FETCH_SIZE, false); |
There was a problem hiding this comment.
I would recommend to name it DEFAULT_FETCH_SIZE
There was a problem hiding this comment.
I'm not clear on your suggestion. Are you suggesting that the url connection parameter be named "DEFAULT_FETCH_SIZE" or as you suggest above "defaultFetchSize". You mention having looked at a few other drivers I've looked too and don't see much consistency. I chose "fetch_size" because that seemed consistent with the other parameters. Would "default_fetch_size" be agreeable?
There was a problem hiding this comment.
yes, default_fetch_size for the jdbc property, which means the enum become DEFAULT_FETCH_SIZE (that was my point :) )
|
@kminder Do you think it would be possible to get this change in by the end of this week? |
|
@kminder We are planning to release Avatica 1.13.0 soon and would love to get this in for the release. Would you be able to take another look at this PR? |
asfgit
force-pushed
the
master
branch
5 times, most recently
from
ff37d89
to
a8617e0
Compare
risdenk
changed the title
F21
force-pushed
the
master
branch
2 times, most recently
from
0640c66
to
d52c203
Compare
vlsi
force-pushed
the
master
branch
3 times, most recently
from
d90fb8c
to
92045d0
Compare
|
Hi, is there a reason this was never merged? We're currently struggling with this and would love to see this make it into the next release. Thanks! |
|
@zacMode Would you be interested in taking over this PR? If so, please go ahead and open a new PR based on this and I can close out this one. There's further discussion on JIRA as well: https://issues.apache.org/jira/browse/CALCITE-2322 |
I'm open to it. I've read over this PR and the corresponding JIRA ticket, but It's not clear what's left to do? |
|
I think there's currently some discussion over the use of camelCase vs snake_case. As the last comment in JIRA is almost 3 years old, I think it would be best to ping the JIRA ticket to confirm whether camelCase or snake_case should be used and go from there. |
|
Got it, will do. Thanks! |
|
Closing this PR as there is a follow up on #148. |
Adds the support for both a URL connection param fetch_size and the JDBD Statement API setFetchSize. This can be used to tune performance for queries that return a large number of rows.