Skip to content

SQL: Fix getTime() methods in JDBC #40484

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

Merged
merged 3 commits into from
Mar 27, 2019
Merged

SQL: Fix getTime() methods in JDBC #40484

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4867899
SQL: Fix getTime() methods in JDBC
matriv Mar 26, 2019
60ca0b4
Remove @Repeat
matriv Mar 26, 2019
6021f09
Address comments
matriv Mar 27, 2019
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
25 changes: 5 additions & 20 deletions x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/JdbcDateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.sql.Date;
import java.sql.Time;
import java.sql.Timestamp;
import java.time.LocalDate;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeFormatterBuilder;
Expand All @@ -27,10 +28,9 @@
*/
final class JdbcDateUtils {

private JdbcDateUtils() {
}
private JdbcDateUtils() {}

private static final long DAY_IN_MILLIS = 60 * 60 * 24 * 1000L;
private static final LocalDate EPOCH = LocalDate.of(1970, 1, 1);

static final DateTimeFormatter ISO_WITH_MILLIS = new DateTimeFormatterBuilder()
.parseCaseInsensitive()
Expand Down Expand Up @@ -58,20 +58,9 @@ static Date asDate(String date) {
return new Date(zdt.toLocalDate().atStartOfDay(zdt.getZone()).toInstant().toEpochMilli());
}

/**
* In contrast to {@link JdbcDateUtils#asDate(String)} here we just want to eliminate
* the date part and just set it to EPOCH (1970-01-1)
*/
static Time asTime(long millisSinceEpoch) {
return new Time(utcMillisRemoveDate(millisSinceEpoch));
}

/**
* In contrast to {@link JdbcDateUtils#asDate(String)} here we just want to eliminate
* the date part and just set it to EPOCH (1970-01-1)
*/
static Time asTime(String date) {
return asTime(asMillisSinceEpoch(date));
ZonedDateTime zdt = asDateTime(date);
return new Time(zdt.toLocalTime().atDate(EPOCH).atZone(zdt.getZone()).toInstant().toEpochMilli());
}

static Timestamp asTimestamp(long millisSinceEpoch) {
Expand All @@ -93,8 +82,4 @@ static <R> R asDateTimeField(Object value, Function<String, R> asDateTimeMethod,
return ctor.apply(((Number) value).longValue());
}
}

private static long utcMillisRemoveDate(long l) {
return l % DAY_IN_MILLIS;
}
}
41 changes: 20 additions & 21 deletions x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.sql.ResultSetMetaData;
import java.sql.SQLException;
import java.time.Instant;
import java.time.LocalDate;
import java.time.ZoneId;
import java.time.ZonedDateTime;
import java.util.ArrayList;
Expand All @@ -37,15 +38,17 @@

import static org.elasticsearch.xpack.sql.action.BasicFormatter.FormatOption.CLI;

public abstract class JdbcTestUtils {
final class JdbcTestUtils {

public static final String SQL_TRACE = "org.elasticsearch.xpack.sql:TRACE";
private JdbcTestUtils() {}

public static final String JDBC_TIMEZONE = "timezone";

public static ZoneId UTC = ZoneId.of("Z");
private static final int MAX_WIDTH = 20;

static final String SQL_TRACE = "org.elasticsearch.xpack.sql:TRACE";
static final String JDBC_TIMEZONE = "timezone";
static final LocalDate EPOCH = LocalDate.of(1970, 1, 1);

public static void logResultSetMetadata(ResultSet rs, Logger logger) throws SQLException {
static void logResultSetMetadata(ResultSet rs, Logger logger) throws SQLException {
ResultSetMetaData metaData = rs.getMetaData();
// header
StringBuilder sb = new StringBuilder();
Expand Down Expand Up @@ -75,9 +78,7 @@ public static void logResultSetMetadata(ResultSet rs, Logger logger) throws SQLE
logger.info(sb.toString());
}

private static final int MAX_WIDTH = 20;

public static void logResultSetData(ResultSet rs, Logger log) throws SQLException {
static void logResultSetData(ResultSet rs, Logger log) throws SQLException {
ResultSetMetaData metaData = rs.getMetaData();
StringBuilder sb = new StringBuilder();
StringBuilder column = new StringBuilder();
Expand All @@ -86,32 +87,30 @@ public static void logResultSetData(ResultSet rs, Logger log) throws SQLExceptio

while (rs.next()) {
sb.setLength(0);
for (int i = 1; i <= columns; i++) {
column.setLength(0);
if (i > 1) {
sb.append(" | ");
}
sb.append(trimOrPad(column.append(rs.getString(i))));
}
rowAsString(rs, sb, column, columns);
log.info(sb);
}
}

public static String resultSetCurrentData(ResultSet rs) throws SQLException {
static String resultSetCurrentData(ResultSet rs) throws SQLException {
ResultSetMetaData metaData = rs.getMetaData();
StringBuilder column = new StringBuilder();

int columns = metaData.getColumnCount();

StringBuilder sb = new StringBuilder();
rowAsString(rs, sb, column, columns);
return sb.toString();
}

private static void rowAsString(ResultSet rs, StringBuilder sb, StringBuilder column, int columns) 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 signature seems incorrect column seems to be used only locally and sb can be returned by the method - passing them as arguments doesn't offer any value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix, just used the automatic refactoring.

for (int i = 1; i <= columns; i++) {
column.setLength(0);
if (i > 1) {
sb.append(" | ");
}
sb.append(trimOrPad(column.append(rs.getString(i))));
}
return sb.toString();
}

private static StringBuilder trimOrPad(StringBuilder buffer) {
Expand Down Expand Up @@ -153,7 +152,7 @@ public static void logLikeCLI(ResultSet rs, Logger logger) throws SQLException {
logger.info("\n" + formatter.formatWithHeader(cols, data));
}

public static String of(long millis, String zoneId) {
static String of(long millis, String zoneId) {
return StringUtils.toString(ZonedDateTime.ofInstant(Instant.ofEpochMilli(millis), ZoneId.of(zoneId)));
}

Expand All @@ -165,7 +164,7 @@ public static String of(long millis, String zoneId) {
* folders in the file-system (typically IDEs) or
* inside jars (gradle).
*/
public static List<URL> classpathResources(String pattern) throws Exception {
static List<URL> classpathResources(String pattern) throws Exception {
while (pattern.startsWith("/")) {
pattern = pattern.substring(1);
}
Expand Down Expand Up @@ -234,4 +233,4 @@ static Tuple<String, String> pathAndName(String string) {
}
return new Tuple<>(folder, file);
}
}
}
17 changes: 13 additions & 4 deletions ...ck/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.sql.qa.jdbc;

import com.carrotsearch.randomizedtesting.annotations.Repeat;
import org.elasticsearch.client.Request;
import org.elasticsearch.common.CheckedBiFunction;
import org.elasticsearch.common.CheckedConsumer;
Expand Down Expand Up @@ -63,6 +64,7 @@
import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.JDBC_TIMEZONE;
import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.of;

@Repeat(iterations = 100)
public class ResultSetTestCase extends JdbcIntegrationTestCase {

static final Set<String> fieldsNames = Stream.of("test_byte", "test_integer", "test_long", "test_short", "test_double",
Expand Down Expand Up @@ -880,7 +882,7 @@ public void testGettingDateWithoutCalendar() throws Exception {
doWithQuery(SELECT_ALL_FIELDS, (results) -> {
results.next();

ZoneId zoneId = ZoneId.of(timeZoneId);
ZoneId zoneId = getZoneFromOffset(randomLongDate);
java.sql.Date expectedDate = new java.sql.Date(
ZonedDateTime.ofInstant(Instant.ofEpochMilli(randomLongDate), zoneId)
.toLocalDate().atStartOfDay(zoneId).toInstant().toEpochMilli());
Expand Down Expand Up @@ -939,11 +941,14 @@ public void testGettingTimeWithoutCalendar() throws Exception {
});
Long randomLongDate = randomNonNegativeLong();
indexSimpleDocumentWithTrueValues(randomLongDate);

doWithQuery(SELECT_ALL_FIELDS, (results) -> {
results.next();

java.sql.Time expectedTime = new java.sql.Time(randomLongDate % 86400000L);
ZoneId zoneId = getZoneFromOffset(randomLongDate);
java.sql.Time expectedTime = new java.sql.Time(
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be extracted in a utils class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't do it as it's used only once here, but I can extract it.

ZonedDateTime.ofInstant(Instant.ofEpochMilli(randomLongDate), zoneId)
.toLocalTime().atDate(JdbcTestUtils.EPOCH).atZone(zoneId).toInstant().toEpochMilli());

assertEquals(expectedTime, results.getTime("test_date"));
assertEquals(expectedTime, results.getTime(9));
Expand All @@ -953,7 +958,7 @@ public void testGettingTimeWithoutCalendar() throws Exception {
validateErrorsForTimeTestsWithoutCalendar(results::getTime);
});
}

public void testGettingTimeWithCalendar() throws Exception {
createIndex("test");
updateMappingForNumericValuesTests("test");
Expand Down Expand Up @@ -1748,4 +1753,8 @@ private Connection esWithLeniency(boolean multiValueLeniency) throws SQLExceptio
private String asDateString(long millis) {
return of(millis, timeZoneId);
}

private ZoneId getZoneFromOffset(Long randomLongDate) {
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 think this reveals an issue with how we send the respond from the server.
Shouldn't we keep for example Australia/Tasmania instead of +10:00`?
@costin @astefan what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+10:00 is static for any timestamp, but Australia/Tasmania offset might change (cause of test failure before).

Copy link
Member

Choose a reason for hiding this comment

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

ZoneId is better I would argue though at the same time, if this is a cause for failure it might mean that the offset rules on the server are different than that on the client which is an issue on itself.

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of agree that +10:00 is ambiguous, but can there be an actual issue here?
Different offset rules on server and on the client can probably happen when different Java versions are used (and offset/timezone impl. differ between the two).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "issue" is that +10:00 is static, it doesn't include DST and it doesn't include any changes in the offset of a timezone (in our example: Australia/Tasmania) that can happen over the years for certain timezones. So if the client would further need to process the result "looses" this info from the result returned, but it still poccesses the Australia/Tasmania since this was the timezone string it set as connection/REST property.

return ZoneId.of(ZoneId.of(timeZoneId).getRules().getOffset(Instant.ofEpochMilli(randomLongDate)).toString());
}
}