[CALCITE-2322] Add fetch row count support#148
Conversation
| HostnameVerification.class, false), | ||
|
|
||
| /** The number of rows to fetch per call, default is 100 rows. */ | ||
| FETCH_ROW_COUNT("fetch_row_count", Type.NUMBER, AvaticaStatement.DEFAULT_FETCH_SIZE, false); |
There was a problem hiding this comment.
What do you think of default_fetch_size?
There was a problem hiding this comment.
I would second @julianhyde 's feedback here:
not "default_fetch_row_count" (because it is understood that this is a default that can be overridden per-statement; furthermore, there is a default default, which is 100);
vlsi
left a comment
vlsi
left a comment
There was a problem hiding this comment.
LGTM modulo the blank line in AvaticaStatement
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
Overall LGTM, is it possible to add some unit tests verifying the default and overriden values?
| private class FetchIterator implements Iterator<Object> { | ||
| private final AvaticaStatement stmt; | ||
| private final QueryState state; | ||
| private final int fetchRowCount; |
There was a problem hiding this comment.
Should this be fetchSizeRows as per the property or there is a reason that the naming is different?
There was a problem hiding this comment.
+1 for using the same terminology
julianhyde
force-pushed
the
master
branch
2 times, most recently
from
a9723ff to
ef277ff
Compare
joshelser
left a comment
joshelser
left a comment
There was a problem hiding this comment.
Nit-picking over some naming, but the change makes sense (and is likely an oversight).
Did you try to mock out enough of this such that we could write a unit test? I can imagine this would be not very straightforward.
| private class FetchIterator implements Iterator<Object> { | ||
| private final AvaticaStatement stmt; | ||
| private final QueryState state; | ||
| private final int fetchRowCount; |
There was a problem hiding this comment.
+1 for using the same terminology
| HostnameVerification.class, false), | ||
|
|
||
| /** The number of rows to fetch per call, default is 100 rows. */ | ||
| FETCH_SIZE_ROWS("fetch_size_rows", Type.NUMBER, AvaticaStatement.DEFAULT_FETCH_SIZE, false); |
There was a problem hiding this comment.
Playing Devil's Advocate: if Avatica is focused as a JDBC abstraction layer, should we just call the client configuration property fetch_size instead of fetch_size_rows? I think the javadoc here and in ConnectionConfig appropriately explains what fetchSize means.
There was a problem hiding this comment.
Agree. The property should be called fetch_size. JDK has a method 'ResultSet.setFetchSize(int rows)' and so 'fetch size' is unambiguously a row count.
|
Hello, what is holding up this feature being merged? It would be very useful. |
8 participants
This resurrects #49, updating @kminder 's existing work per guidance from CALCITE-2322.
The purpose of this change is to add support for configurable "fetch size"/ "fetch row count". Currently, setting fetchSize has no effect, because avatica always uses a hardcoded default value. This PR resolves that issue.