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

SNOW-928973: Date field is returning one day less when getting through getString method #1715

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
963cf00
Replace sleep with awaitility to reduce overall test time
sfc-gh-ext-simba-nl Apr 6, 2024
3fbc4e8
fix formatting
sfc-gh-ext-simba-nl Apr 6, 2024
725197e
Merge branch 'master' into SNOW-1213116-reduce-test-sleep-time
sfc-gh-ext-simba-nl Apr 8, 2024
df6a02b
Change atMost in await
sfc-gh-ext-simba-nl Apr 9, 2024
a504800
Have consistent behaviour with dates between different APIs
sfc-gh-ext-simba-nl Apr 10, 2024
7ed7309
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Apr 18, 2024
12d32a7
Add JDBC_USE_HARDCODED_TIMEZONE parameter to retain previous bahaviour
sfc-gh-ext-simba-nl Apr 18, 2024
49657d8
Fix typo to change parameter to not required
sfc-gh-ext-simba-nl Apr 18, 2024
88637af
fix formatting
sfc-gh-ext-simba-nl Apr 18, 2024
f5f42be
Fix tests, add null checks for context
sfc-gh-ext-simba-nl Apr 18, 2024
757600c
Fix test null issue
sfc-gh-ext-simba-nl Apr 18, 2024
9b5def4
Fix tests, add null checks
sfc-gh-ext-simba-nl Apr 19, 2024
ec95f97
Fix formatting
sfc-gh-ext-simba-nl Apr 19, 2024
7009b47
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Apr 23, 2024
8ab3486
Fix tests
sfc-gh-ext-simba-nl Apr 23, 2024
82296b7
Fix formatting
sfc-gh-ext-simba-nl Apr 23, 2024
64607e2
revert change for datetimeconverter
sfc-gh-ext-simba-nl Apr 23, 2024
6cc5d59
Revert previous change in SFBaseResultSet
sfc-gh-ext-simba-nl Apr 30, 2024
d666ee0
Merge master
sfc-gh-ext-simba-nl May 28, 2024
d9727f4
Fix formatting
sfc-gh-ext-simba-nl May 28, 2024
79ce061
fix issue with setting wrong tz
sfc-gh-ext-simba-nl May 29, 2024
5848c20
Make change to set tz to JVM and check to use session tz downstream
sfc-gh-ext-simba-nl May 30, 2024
1a34aaa
Fix formatting
sfc-gh-ext-simba-nl May 30, 2024
fb9d4fa
Fix test errors
sfc-gh-ext-simba-nl Jun 3, 2024
476d5d9
Fix test in LoaderIT
sfc-gh-ext-simba-nl Jun 3, 2024
93de961
Fix property name in test
sfc-gh-ext-simba-nl Jun 3, 2024
36dd932
Fix tests
sfc-gh-ext-simba-nl Jun 6, 2024
c757d1d
Set session timezone to false for timezone test case
sfc-gh-ext-simba-nl Jun 6, 2024
6630e40
Fix session to true
sfc-gh-ext-simba-nl Jun 6, 2024
95ddcb4
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Jun 7, 2024
820580e
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Jun 11, 2024
4feb2f4
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Jun 18, 2024
4819e90
Merge master
sfc-gh-ext-simba-nl Jul 5, 2024
f9d1269
Fix styling
sfc-gh-ext-simba-nl Jul 5, 2024
3080186
Fix failing test
sfc-gh-ext-simba-nl Jul 5, 2024
b975372
Remove setting JDBC_FORMAT_DATE_WITH_TIMEZONE in tests, fix default t…
sfc-gh-ext-simba-nl Jul 10, 2024
5d4c15d
Add back setting JDBC_FORMAT_DATE_WITH_TIMEZONE to false
sfc-gh-ext-simba-nl Jul 10, 2024
5aebbd5
Refactor testTimezoneDates in DateConversionTest
sfc-gh-ext-simba-nl Jul 10, 2024
d5e8913
Merge branch 'master' into SNOW-928973-incorrect-getString-date-field…
sfc-gh-ext-simba-nl Jul 10, 2024
95789b7
Fix test
sfc-gh-ext-simba-nl Jul 11, 2024
362981e
Rename property to JDBC_DEFAULT_FORMAT_DATE_WITH_TIMEZONE, revert tim…
sfc-gh-ext-simba-nl Jul 15, 2024
31f31de
Revert changes in tests along with the BCR revert
sfc-gh-ext-simba-nl Jul 16, 2024
6c47f1f
Update variable names for defaultFormatDateWithTimezone, add connecti…
sfc-gh-ext-simba-nl Jul 16, 2024
773d502
Fix styling
sfc-gh-ext-simba-nl Jul 16, 2024
90d0d65
Make descriptions more clear
sfc-gh-ext-simba-nl Jul 18, 2024
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
18 changes: 18 additions & 0 deletions src/main/java/net/snowflake/client/core/SFBaseSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ public abstract class SFBaseSession {
private boolean enableCombineDescribe;
private boolean clientTelemetryEnabled = false;
private boolean useSessionTimezone;
private boolean defaultFormatDateWithTimezone = true;
private boolean getDateUseNullTimezone = true;
// The server can read array binds from a stage instead of query payload.
// When there as many bind values as this threshold, we should upload them to a stage.
private int arrayBindStageThreshold = 0;
Expand Down Expand Up @@ -679,10 +681,26 @@ public boolean getUseSessionTimezone() {
return useSessionTimezone;
}

public boolean getDefaultFormatDateWithTimezone() {
return defaultFormatDateWithTimezone;
}

public void setUseSessionTimezone(boolean useSessionTimezone) {
this.useSessionTimezone = useSessionTimezone;
}

public void setDefaultFormatDateWithTimezone(boolean defaultFormatDateWithTimezone) {
this.defaultFormatDateWithTimezone = defaultFormatDateWithTimezone;
}

public boolean getGetDateUseNullTimezone() {
return getDateUseNullTimezone;
}

public void setGetDateUseNullTimezone(boolean getDateUseNullTimezone) {
this.getDateUseNullTimezone = getDateUseNullTimezone;
}

public boolean getEnableCombineDescribe() {
return enableCombineDescribe;
}
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/net/snowflake/client/core/SFSession.java
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,18 @@ public void addSFSessionProperty(String propertyName, Object propertyValue) thro
}
break;

case JDBC_DEFAULT_FORMAT_DATE_WITH_TIMEZONE:
if (propertyValue != null) {
setDefaultFormatDateWithTimezone(getBooleanValue(propertyValue));
}
break;

case JDBC_GET_DATE_USE_NULL_TIMEZONE:
if (propertyValue != null) {
setGetDateUseNullTimezone(getBooleanValue(propertyValue));
}
break;

default:
break;
}
Expand Down
11 changes: 10 additions & 1 deletion src/main/java/net/snowflake/client/core/SFSessionProperty.java
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,16 @@ public enum SFSessionProperty {
DISABLE_GCS_DEFAULT_CREDENTIALS("disableGcsDefaultCredentials", false, Boolean.class),

JDBC_ARROW_TREAT_DECIMAL_AS_INT("JDBC_ARROW_TREAT_DECIMAL_AS_INT", false, Boolean.class),
DISABLE_SAML_URL_CHECK("disableSamlURLCheck", false, Boolean.class);

DISABLE_SAML_URL_CHECK("disableSamlURLCheck", false, Boolean.class),

// Used to determine whether to use the previously hardcoded value for the formatter (for
// backwards compatibility) or use the value of JDBC_FORMAT_DATE_WITH_TIMEZONE
JDBC_DEFAULT_FORMAT_DATE_WITH_TIMEZONE(
"JDBC_DEFAULT_FORMAT_DATE_WITH_TIMEZONE", false, Boolean.class),

// Used as a fix for issue SNOW-354859. Remove with snowflake-jdbc version 4.x with BCR changes.
JDBC_GET_DATE_USE_NULL_TIMEZONE("JDBC_GET_DATE_USE_NULL_TIMEZONE", false, Boolean.class);

// property key in string
private String propertyKey;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,23 @@ public class DateConverter extends AbstractArrowVectorConverter {
private DateDayVector dateVector;
private static TimeZone timeZoneUTC = TimeZone.getTimeZone("UTC");

private boolean useDateFormat;

@Deprecated
public DateConverter(ValueVector fieldVector, int columnIndex, DataConversionContext context) {
super(SnowflakeType.DATE.name(), fieldVector, columnIndex, context);
this.dateVector = (DateDayVector) fieldVector;
this.useDateFormat = false;
}

public DateConverter(
ValueVector fieldVector,
int columnIndex,
DataConversionContext context,
boolean useDateFormat) {
super(SnowflakeType.DATE.name(), fieldVector, columnIndex, context);
this.dateVector = (DateDayVector) fieldVector;
this.useDateFormat = useDateFormat;
}

private Date getDate(int index, TimeZone jvmTz, boolean useDateFormat) throws SFException {
Expand Down Expand Up @@ -86,7 +100,11 @@ public BigDecimal toBigDecimal(int index) {

@Override
public Timestamp toTimestamp(int index, TimeZone tz) throws SFException {
Date date = toDate(index, tz, true);
boolean useDateFormat = true;
if (this.context.getSession() != null) {
useDateFormat = getUseDateFormat(true);
}
Date date = toDate(index, tz, useDateFormat);
if (date == null) {
return null;
} else {
Expand All @@ -99,21 +117,21 @@ public String toString(int index) throws SFException {
if (context.getDateFormatter() == null) {
throw new SFException(ErrorCode.INTERNAL_ERROR, "missing date formatter");
}
Date date = getDate(index, timeZoneUTC, false);
Date date = getDate(index, timeZoneUTC, getUseDateFormat(false));
return date == null ? null : ResultUtil.getDateAsString(date, context.getDateFormatter());
}

@Override
public Object toObject(int index) throws SFException {
return toDate(index, TimeZone.getDefault(), false);
return toDate(index, TimeZone.getDefault(), getUseDateFormat(false));
}

@Override
public boolean toBoolean(int index) throws SFException {
if (isNull(index)) {
return false;
}
Date val = toDate(index, TimeZone.getDefault(), false);
Date val = toDate(index, TimeZone.getDefault(), getUseDateFormat(false));
throw new SFException(
ErrorCode.INVALID_VALUE_CONVERT, logicalTypeStr,
SnowflakeUtil.BOOLEAN_STR, val);
Expand All @@ -128,4 +146,12 @@ public static Date getDate(
// Note: use default time zone to match with current getDate() behavior
return ArrowResultUtil.getDate(value, jvmTz, sessionTimeZone);
}

private Boolean getUseDateFormat(Boolean defaultValue) {
return this.context.getSession() == null
? defaultValue
: (this.context.getSession().getDefaultFormatDateWithTimezone()
? defaultValue
: this.useDateFormat);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,11 @@ private static List<ArrowVectorConverter> initConverters(
break;

case DATE:
converters.add(new DateConverter(vector, i, context));
boolean getFormatDateWithTimeZone = false;
if (context.getSession() != null) {
getFormatDateWithTimeZone = context.getSession().getFormatDateWithTimezone();
}
converters.add(new DateConverter(vector, i, context, getFormatDateWithTimeZone));
break;

case FIXED:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,9 @@ protected void raiseSQLExceptionIfResultSetIsClosed() throws SQLException {
@Override
public Date getDate(int columnIndex) throws SQLException {
raiseSQLExceptionIfResultSetIsClosed();
return getDate(columnIndex, (TimeZone) null);
return getDate(
columnIndex,
this.session.getGetDateUseNullTimezone() ? (TimeZone) null : TimeZone.getDefault());
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -231,4 +231,13 @@ public void setPrivateKeyFile(String location, String password) {
public void setTracing(String tracing) {
this.properties.put("tracing", tracing);
}

public void setJDBCDefaultFormatDateWithTimezone(Boolean jdbcDefaultFormatDateWithTimezone) {
this.properties.put(
"JDBC_DEFAULT_FORMAT_DATE_WITH_TIMEZONE", jdbcDefaultFormatDateWithTimezone);
}

public void setGetDateUseNullTimezone(Boolean getDateUseNullTimezone) {
this.properties.put("JDBC_GET_DATE_USE_NULL_TIMEZONE", getDateUseNullTimezone);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,24 @@
import static org.junit.Assert.assertTrue;

import java.sql.Date;
import java.util.Arrays;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Random;
import java.util.Set;
import java.util.TimeZone;
import net.snowflake.client.TestUtil;
import net.snowflake.client.core.SFException;
import net.snowflake.client.core.json.DateTimeConverter;
import org.apache.arrow.memory.BufferAllocator;
import org.apache.arrow.memory.RootAllocator;
import org.apache.arrow.vector.DateDayVector;
import org.apache.arrow.vector.types.Types;
import org.apache.arrow.vector.types.pojo.FieldType;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
Expand Down Expand Up @@ -63,6 +68,34 @@ public DateConverterTest(String tz) {
"2016-04-20"
};

// Map of timezone to ArrowResultUtil.getDate value and hours offset for testTimezoneDates test
// case
Map<String, List<Object>> timezoneDatesData =
new HashMap<String, List<Object>>() {
{
put("UTC", Arrays.asList("2016-04-20", 0));
put("America/Los_Angeles", Arrays.asList("2016-04-20", -7));
put("America/New_York", Arrays.asList("2016-04-20", -4));
put("Pacific/Honolulu", Arrays.asList("2016-04-20", -10));
put("Asia/Singapore", Arrays.asList("2016-04-19", 8));
put("MEZ", Arrays.asList("2016-04-20", 0));
put("MESZ", Arrays.asList("2016-04-20", 0));
}
};

public static final int MILLIS_IN_ONE_HOUR = 3600000;
private TimeZone defaultTimeZone;

@Before
public void getDefaultTimeZone() {
this.defaultTimeZone = TimeZone.getDefault();
}

@After
public void restoreDefaultTimeZone() {
TimeZone.setDefault(defaultTimeZone);
}

@Test
public void testDate() throws SFException {
Map<String, String> customFieldMeta = new HashMap<>();
Expand All @@ -85,7 +118,7 @@ public void testDate() throws SFException {
j++;
}

ArrowVectorConverter converter = new DateConverter(vector, 0, this);
ArrowVectorConverter converter = new DateConverter(vector, 0, this, false);
int rowCount = j;
i = 0;
j = 0;
Expand Down Expand Up @@ -144,7 +177,7 @@ public void testRandomDates() throws SFException {
}
}

ArrowVectorConverter converter = new DateConverter(vector, 0, this);
ArrowVectorConverter converter = new DateConverter(vector, 0, this, false);

for (int i = 0; i < rowCount; i++) {
int intVal = converter.toInt(i);
Expand All @@ -162,4 +195,45 @@ public void testRandomDates() throws SFException {
}
}
}

@Test
public void testTimezoneDates() throws SFException {
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
int testDay = 16911;
Map<String, String> customFieldMeta = new HashMap<>();
customFieldMeta.put("logicalType", "DATE");
// test normal date
FieldType fieldType =
new FieldType(true, Types.MinorType.DATEDAY.getType(), null, customFieldMeta);

DateDayVector vector = new DateDayVector("date", fieldType, allocator);

vector.setSafe(0, testDay);

// Test JDBC_FORMAT_DATE_WITH_TIMEZONE=TRUE with different session timezones
TimeZone.setDefault(TimeZone.getTimeZone("UTC"));
String tz = System.getProperty("user.timezone");
ArrowVectorConverter converter = new DateConverter(vector, 0, this, true);
converter.setUseSessionTimezone(true);
converter.setSessionTimeZone(TimeZone.getTimeZone(tz));
sfc-gh-dprzybysz marked this conversation as resolved.
Show resolved Hide resolved
Object obj = converter.toObject(0);
DateTimeConverter jsonConverter =
new DateTimeConverter(
TimeZone.getTimeZone(tz), this.getSession(), 0, true, false, true, true);
Date jsonDate =
jsonConverter.getDate(Integer.toString(testDay), 91, 91, TimeZone.getTimeZone(tz), 0);
Object utcObj =
ArrowResultUtil.getDate(testDay, TimeZone.getTimeZone("UTC"), TimeZone.getTimeZone(tz));

List<Object> testValues = this.timezoneDatesData.get(tz);
assertTrue(testValues.get(0) instanceof String);
assertTrue(testValues.get(1) instanceof Integer);
assertThat(obj.toString(), is("2016-04-20"));
assertThat(jsonDate.toString(), is(obj.toString()));
assertThat(utcObj.toString(), is(testValues.get(0)));
assertThat(
((Date) obj).getTime(),
is(((Date) utcObj).getTime() + ((Integer) testValues.get(1) * MILLIS_IN_ONE_HOUR)));

vector.clear();
}
}
29 changes: 16 additions & 13 deletions src/test/java/net/snowflake/client/loader/LoaderIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.sql.Time;
import java.util.Arrays;
import java.util.Calendar;
Expand Down Expand Up @@ -142,19 +144,20 @@ public void testLoadTime() throws Exception {
errorMessage = listener.getErrors().get(0).getException().toString();
}
assertThat(String.format("Error: %s", errorMessage), listener.getErrorCount(), equalTo(0));
ResultSet rs =
testConnection
.createStatement()
.executeQuery(String.format("SELECT c1, c2 FROM %s LIMIT 1", tableName));
rs.next();
Time rsTm = rs.getTime(1);
Date rsDt = rs.getDate(2);
assertThat("Time column didn't match", rsTm, equalTo(tm));

Calendar cal = cutOffTimeFromDate(dt);
long dtEpoch = cal.getTimeInMillis();
long rsDtEpoch = rsDt.getTime();
assertThat("Date column didn't match", rsDtEpoch, equalTo(dtEpoch));
try (Statement statement = testConnection.createStatement()) {
try (ResultSet rs =
statement.executeQuery(String.format("SELECT c1, c2 FROM %s LIMIT 1", tableName))) {
assertTrue(rs.next());
Time rsTm = rs.getTime(1);
Date rsDt = rs.getDate(2);
assertThat("Time column didn't match", rsTm, equalTo(tm));

Calendar cal = cutOffTimeFromDate(dt);
long dtEpoch = cal.getTimeInMillis();
long rsDtEpoch = rsDt.getTime();
assertThat("Date column didn't match", rsDtEpoch, equalTo(dtEpoch));
}
}
} finally {
testConnection.createStatement().execute(String.format("DROP TABLE IF EXISTS %s", tableName));
}
Expand Down
Loading