Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CALCITE-2322] Add fetch size support to connection url and JDBC state… #49

Closed
wants to merge 1 commit into from
Closed

[CALCITE-2322] Add fetch size support to connection url and JDBC state… #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/calcite/avatica/AvaticaStatement.java
View file
Open in desktop
Expand Up @@ -36,6 +36,7 @@
*/
public abstract class AvaticaStatement
implements Statement {

/** The default value for {@link Statement#getFetchSize()}. */
public static final int DEFAULT_FETCH_SIZE = 100;

Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the default value set for the field declaration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the value is both set in the field declaration and in the constructor, just suggesting to remove the default value in the field initialization...

this.signature = signature;
this.closed = false;
if (h == null) {
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/org/apache/calcite/avatica/BuiltInConnectionProperty.java
View file
Open in desktop
Expand Up @@ -77,7 +77,10 @@ public enum BuiltInConnectionProperty implements ConnectionProperty {
TRUSTSTORE_PASSWORD("truststore_password", Type.STRING, null, false),

HOSTNAME_VERIFICATION("hostname_verification", Type.ENUM, HostnameVerification.STRICT,
HostnameVerification.class, false);
HostnameVerification.class, false),

/** Fetch size limit, default is 100 rows. */
FETCH_SIZE("fetch_size", Type.NUMBER, AvaticaStatement.DEFAULT_FETCH_SIZE, false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to name it DEFAULT_FETCH_SIZE

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, default_fetch_size for the jdbc property, which means the enum become DEFAULT_FETCH_SIZE (that was my point :) )


private final String camelName;
private final Type type;
Expand Down
2 changes: 2 additions & 0 deletions core/src/main/java/org/apache/calcite/avatica/ConnectionConfig.java
View file
Open in desktop
Expand Up @@ -56,6 +56,8 @@ public interface ConnectionConfig {
String truststorePassword();
/** @see BuiltInConnectionProperty#HOSTNAME_VERIFICATION */
HostnameVerification hostnameVerification();
/** @see BuiltInConnectionProperty#FETCH_SIZE */
int fetchSize();
}

// End ConnectionConfig.java
4 changes: 4 additions & 0 deletions core/src/main/java/org/apache/calcite/avatica/ConnectionConfigImpl.java
View file
Open in desktop
Expand Up @@ -111,6 +111,10 @@ public HostnameVerification hostnameVerification() {
.getEnum(HostnameVerification.class);
}

public int fetchSize() {
return BuiltInConnectionProperty.FETCH_SIZE.wrap(properties).getInt();
}

/** Converts a {@link Properties} object containing (name, value)
* pairs into a map whose keys are
* {@link org.apache.calcite.avatica.InternalProperty} objects.
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/calcite/avatica/MetaImpl.java
View file
Open in desktop
Expand Up @@ -1603,7 +1603,7 @@ private void moveNext() {
}
try {
// currentOffset updated after element is read from `rows` iterator
frame = fetch(stmt.handle, currentOffset, AvaticaStatement.DEFAULT_FETCH_SIZE);
frame = fetch(stmt.handle, currentOffset, stmt.getFetchSize());
} catch (NoSuchStatementException e) {
resetStatement();
// re-fetch the batch where we left off
Expand Down