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.

Sign up for GitHub

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

JDBC driver prepared statement set* methods #31494

Merged
merged 9 commits into from
Jun 27, 2018
Merged

JDBC driver prepared statement set* methods #31494

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8e80894
Added setObject functionality and tests for it
astefan Jun 15, 2018
2746d2e
Minor changes
astefan Jun 15, 2018
9022d66
Added the Calendar conversion to UTC for time based setters. Also, ad…
astefan Jun 20, 2018
31cbdca
Removed DataType.javaName redundant information
astefan Jun 20, 2018
8f283da
Merge remote-tracking branch 'remotes/origin/master' into jdbc_driver…
astefan Jun 21, 2018
4a076cc
Fixed spacing on one line
astefan Jun 21, 2018
0550416
Fixed `gradle check` complaints
astefan Jun 21, 2018
2e21907
Cosmetic changes to test methods. Changed the way dates using Calenda…
astefan Jun 22, 2018
04136cf
Readability improvements
astefan Jun 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
200 changes: 173 additions & 27 deletions ...n/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcPreparedStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/
package org.elasticsearch.xpack.sql.jdbc.jdbc;

import org.elasticsearch.xpack.sql.type.DataType;

import java.io.InputStream;
import java.io.Reader;
import java.math.BigDecimal;
Expand All @@ -24,10 +26,20 @@
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.sql.SQLXML;
import java.sql.Struct;
import java.sql.Time;
import java.sql.Timestamp;
import java.sql.Types;
import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.time.OffsetDateTime;
import java.time.OffsetTime;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Calendar;
import java.util.List;
import java.util.Locale;

class JdbcPreparedStatement extends JdbcStatement implements PreparedStatement {
final PreparedQuery query;
Expand Down Expand Up @@ -74,67 +86,67 @@ public void setNull(int parameterIndex, int sqlType) throws SQLException {

@Override
public void setBoolean(int parameterIndex, boolean x) throws SQLException {
setParam(parameterIndex, x, Types.BOOLEAN);
setObject(parameterIndex, x, Types.BOOLEAN);
}

@Override
public void setByte(int parameterIndex, byte x) throws SQLException {
setParam(parameterIndex, x, Types.TINYINT);
setObject(parameterIndex, x, Types.TINYINT);
}

@Override
public void setShort(int parameterIndex, short x) throws SQLException {
setParam(parameterIndex, x, Types.SMALLINT);
setObject(parameterIndex, x, Types.SMALLINT);
}

@Override
public void setInt(int parameterIndex, int x) throws SQLException {
setParam(parameterIndex, x, Types.INTEGER);
setObject(parameterIndex, x, Types.INTEGER);
}

@Override
public void setLong(int parameterIndex, long x) throws SQLException {
setParam(parameterIndex, x, Types.BIGINT);
setObject(parameterIndex, x, Types.BIGINT);
}

@Override
public void setFloat(int parameterIndex, float x) throws SQLException {
setParam(parameterIndex, x, Types.REAL);
setObject(parameterIndex, x, Types.REAL);
}

@Override
public void setDouble(int parameterIndex, double x) throws SQLException {
setParam(parameterIndex, x, Types.DOUBLE);
setObject(parameterIndex, x, Types.DOUBLE);
}

@Override
public void setBigDecimal(int parameterIndex, BigDecimal x) throws SQLException {
throw new SQLFeatureNotSupportedException("BigDecimal not supported");
setObject(parameterIndex, x, Types.BIGINT);
}

@Override
public void setString(int parameterIndex, String x) throws SQLException {
setParam(parameterIndex, x, Types.VARCHAR);
setObject(parameterIndex, x, Types.VARCHAR);
}

@Override
public void setBytes(int parameterIndex, byte[] x) throws SQLException {
throw new UnsupportedOperationException("Bytes not implemented yet");
setObject(parameterIndex, x);
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't Types specified as in the methods above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

}

@Override
public void setDate(int parameterIndex, Date x) throws SQLException {
throw new UnsupportedOperationException("Date/Time not implemented yet");
setObject(parameterIndex, x);
}

@Override
public void setTime(int parameterIndex, Time x) throws SQLException {
throw new UnsupportedOperationException("Date/Time not implemented yet");
setObject(parameterIndex, x);
}

@Override
public void setTimestamp(int parameterIndex, Timestamp x) throws SQLException {
throw new UnsupportedOperationException("Date/Time not implemented yet");
setObject(parameterIndex, x);
}

@Override
Expand All @@ -161,12 +173,22 @@ public void clearParameters() throws SQLException {

@Override
public void setObject(int parameterIndex, Object x, int targetSqlType) throws SQLException {
throw new UnsupportedOperationException("Object not implemented yet");
// the value of scaleOrLength parameter doesn't matter, as it's not used in the called method below
setObject(parameterIndex, x, targetSqlType, 0);
}

@Override
public void setObject(int parameterIndex, Object x) throws SQLException {
throw new SQLFeatureNotSupportedException("CharacterStream not supported");
if (x == null) {
setParam(parameterIndex, null, Types.NULL);
return;
}

// check also here the unsupported types so that any unsupported interfaces ({@code java.sql.Struct},
// {@code java.sql.Array} etc) will generate the correct exception message. Otherwise, the method call
// {@code TypeConverter.fromJavaToJDBC(x.getClass())} will report the implementing class as not being supported.
checkKnownUnsupportedTypes(x);
setObject(parameterIndex, x, TypeConverter.fromJavaToJDBC(x.getClass()).getVendorTypeNumber(), 0);
}

@Override
Expand All @@ -181,22 +203,22 @@ public void setCharacterStream(int parameterIndex, Reader reader, int length) th

@Override
public void setRef(int parameterIndex, Ref x) throws SQLException {
throw new SQLFeatureNotSupportedException("Ref not supported");
setObject(parameterIndex, x);
}

@Override
public void setBlob(int parameterIndex, Blob x) throws SQLException {
throw new SQLFeatureNotSupportedException("Blob not supported");
setObject(parameterIndex, x);
}

@Override
public void setClob(int parameterIndex, Clob x) throws SQLException {
throw new SQLFeatureNotSupportedException("Clob not supported");
setObject(parameterIndex, x);
}

@Override
public void setArray(int parameterIndex, Array x) throws SQLException {
throw new SQLFeatureNotSupportedException("Array not supported");
setObject(parameterIndex, x);
}

@Override
Expand All @@ -206,17 +228,41 @@ public ResultSetMetaData getMetaData() throws SQLException {

@Override
public void setDate(int parameterIndex, Date x, Calendar cal) throws SQLException {
throw new UnsupportedOperationException("Dates not implemented yet");
if (cal == null) {
setDate(parameterIndex, x);
return;
}
if (checkNull(parameterIndex, x, Types.TIMESTAMP)) {
return;
}

setDate(parameterIndex, new Date(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)));
Copy link
Member

Choose a reason for hiding this comment

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

This should call setObject directly not setDate (with 2 params) which effectively is setDate(paramIndex,x,NULL).
The 2 arg method should invoke the 3 arg one not vice-versa since the former is a subset of the latter.

}

@Override
public void setTime(int parameterIndex, Time x, Calendar cal) throws SQLException {
throw new UnsupportedOperationException("Dates not implemented yet");
if (cal == null) {
setTime(parameterIndex, x);
return;
}
if (checkNull(parameterIndex, x, Types.TIMESTAMP)) {
return;
}

setTime(parameterIndex, new Time(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)));
Copy link
Member

Choose a reason for hiding this comment

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

Same comments as above.

}

@Override
public void setTimestamp(int parameterIndex, Timestamp x, Calendar cal) throws SQLException {
throw new UnsupportedOperationException("Dates not implemented yet");
if (cal == null) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

setTimestamp(parameterIndex, x);
return;
}
if (checkNull(parameterIndex, x, Types.TIMESTAMP)) {
return;
}

setTimestamp(parameterIndex, new Timestamp(TypeConverter.convertFromCalendarToUTC(x.getTime(), cal)));
}

@Override
Expand All @@ -226,7 +272,7 @@ public void setNull(int parameterIndex, int sqlType, String typeName) throws SQL

@Override
public void setURL(int parameterIndex, URL x) throws SQLException {
throw new SQLFeatureNotSupportedException("Datalink not supported");
setObject(parameterIndex, x);
}

@Override
Expand All @@ -236,7 +282,7 @@ public ParameterMetaData getParameterMetaData() throws SQLException {

@Override
public void setRowId(int parameterIndex, RowId x) throws SQLException {
throw new SQLFeatureNotSupportedException("RowId not supported");
setObject(parameterIndex, x);
}

@Override
Expand All @@ -251,7 +297,7 @@ public void setNCharacterStream(int parameterIndex, Reader value, long length) t

@Override
public void setNClob(int parameterIndex, NClob value) throws SQLException {
throw new SQLFeatureNotSupportedException("NClob not supported");
setObject(parameterIndex, value);
}

@Override
Expand All @@ -271,12 +317,104 @@ public void setNClob(int parameterIndex, Reader reader, long length) throws SQLE

@Override
public void setSQLXML(int parameterIndex, SQLXML xmlObject) throws SQLException {
throw new SQLFeatureNotSupportedException("SQLXML not supported");
setObject(parameterIndex, xmlObject);
}

@Override
public void setObject(int parameterIndex, Object x, int targetSqlType, int scaleOrLength) throws SQLException {
throw new UnsupportedOperationException("Object not implemented yet");
checkOpen();

JDBCType targetJDBCType;
try {
// this is also a way to check early for the validity of the desired sql type
targetJDBCType = JDBCType.valueOf(targetSqlType);
} catch (IllegalArgumentException e) {
throw new SQLException(e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

The exception should be more precise, either SQLDataException or its parent SQLNonTransientException.
The same comment applies for the rest of the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

}

// set the null value on the type and exit
if (x == null) {
setParam(parameterIndex, null, targetSqlType);
return;
}

checkKnownUnsupportedTypes(x);
if (x instanceof Boolean
|| x instanceof Byte
|| x instanceof Short
|| x instanceof Integer
|| x instanceof Long
|| x instanceof Float
|| x instanceof Double
|| x instanceof String) {
try {
setParam(parameterIndex,
TypeConverter.convert(x, TypeConverter.fromJavaToJDBC(x.getClass()), DataType.fromJdbcTypeToJava(targetJDBCType)),
targetSqlType);
} catch (ClassCastException cce) {
throw new SQLException("Unable to convert " + x.getClass().getName() + " to " + targetJDBCType, cce);
}
} else if (x instanceof Timestamp
|| x instanceof Calendar
|| x instanceof java.util.Date
|| x instanceof LocalDateTime
|| x instanceof Date
|| x instanceof Time) {
if (targetJDBCType == JDBCType.TIMESTAMP ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you do these with a single instanceof? rather than check each one twice? Maybe call setParam(parameterIndex, dateToSet, Types.TIMESTAMP); on the result each time and return?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nik9000 Haven't found a better/easier-to-follow way of refactoring this block of code mostly due to the inner if-else for the target jdbc type. I have gotten rid of the else if bunch though.

// converting to {@code java.util.Date} because this is the type supported by {@code XContentBuilder} for serialization
java.util.Date dateToSet;
if (x instanceof Timestamp) {
dateToSet = new java.util.Date(((Timestamp) x).getTime());
Copy link
Member

Choose a reason for hiding this comment

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

That's round the fractional milliseconds. Are we sure that is right? I think it is worth a comment if we are. If not, worth thinking about some more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nik9000 I did it like this since ES deals with dates in milliseconds since epoch. The parameters will be serialized using a java.util.Date when being sent to ES in queries.

Copy link
Member

Choose a reason for hiding this comment

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

Right. I think maybe this is worth playing with some more and documenting any strange things that come out of it. Though I think it'd be fine to do in a followup.

} else if (x instanceof Calendar) {
dateToSet = ((Calendar) x).getTime();
} else if (x instanceof java.util.Date) {
dateToSet = (java.util.Date) x;
} else if (x instanceof LocalDateTime){
LocalDateTime ldt = (LocalDateTime) x;
Calendar cal = getDefaultCalendar();
cal.set(ldt.getYear(), ldt.getMonthValue() - 1, ldt.getDayOfMonth(), ldt.getHour(), ldt.getMinute(), ldt.getSecond());

dateToSet = cal.getTime();
} else if (x instanceof Date) {
dateToSet = TypeConverter.convertDate(((Date) x).getTime(), getDefaultCalendar());
} else {
dateToSet = TypeConverter.convertDate(((Time) x).getTime(), getDefaultCalendar());
}

setParam(parameterIndex, dateToSet, Types.TIMESTAMP);
} else if (targetJDBCType == JDBCType.VARCHAR) {
setParam(parameterIndex, String.valueOf(x), Types.VARCHAR);
} else {
// anything else other than VARCHAR and TIMESTAMP is not supported in this JDBC driver
throw new SQLFeatureNotSupportedException(
"Conversion from type " + x.getClass().getName() + " to " + targetJDBCType + " not supported");
}
} else if (x instanceof byte[]) {
if (targetJDBCType != JDBCType.VARBINARY) {
throw new SQLFeatureNotSupportedException(
"Conversion from type byte[] to " + targetJDBCType + " not supported");
}
setParam(parameterIndex, x, Types.VARBINARY);
} else {
throw new SQLFeatureNotSupportedException(
"Conversion from type " + x.getClass().getName() + " to " + targetJDBCType + " not supported");
}
}

private void checkKnownUnsupportedTypes(Object x) throws SQLFeatureNotSupportedException {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it'd be cleaner to have this be the result of falling off the end of an if statement chain.

List<Class<?>> unsupportedTypes = new ArrayList<Class<?>>(Arrays.asList(Struct.class, Array.class, SQLXML.class,
RowId.class, Ref.class, Blob.class, NClob.class, Clob.class, LocalDate.class, LocalTime.class,
OffsetTime.class, OffsetDateTime.class, URL.class, BigDecimal.class));

for (Class<?> clazz:unsupportedTypes) {
if (clazz.isAssignableFrom(x.getClass())) {
throw new SQLFeatureNotSupportedException("Objects of type " + clazz.getName() + " are not supported");
}
}
}

private Calendar getDefaultCalendar() {
return Calendar.getInstance(cfg.timeZone(), Locale.ROOT);
Copy link
Member

Choose a reason for hiding this comment

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

This used to be a final field, why was it moved to a method?

}

@Override
Expand Down Expand Up @@ -378,4 +516,12 @@ public boolean execute(String sql, String[] columnNames) throws SQLException {
public long executeLargeUpdate() throws SQLException {
throw new SQLFeatureNotSupportedException("Batching not supported");
}

private boolean checkNull(int parameterIndex, Object o, int type) throws SQLException {
Copy link
Member

Choose a reason for hiding this comment

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

The method name seems incorrect since it's not a check but a set. Something like setIfNull or handleNull is more appropriate.

if (o == null) {
setNull(parameterIndex, type);
return true;
}
return false;
}
}
15 changes: 5 additions & 10 deletions ...ck/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/jdbc/JdbcResultSet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,17 +359,12 @@ private <T> T convert(int columnIndex, Class<T> type) throws SQLException {
return null;
}

if (type != null && type.isInstance(val)) {
try {
return type.cast(val);
} catch (ClassCastException cce) {
throw new SQLException("unable to convert column " + columnIndex + " to " + type, cce);
}
}

JDBCType columnType = cursor.columns().get(columnIndex - 1).type;

return TypeConverter.convert(val, columnType, type);
try {
return TypeConverter.convert(val, columnType, type);
} catch (ClassCastException cce) {
throw new SQLException("unable to convert column " + columnIndex + " to " + type, cce);
}
}

@Override
Expand Down
Loading