Skip to content

Fix incorrect date handling in Phoenix 4 & 5 #10749

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
ebyhr merged 1 commit into from
Jan 31, 2022
Merged

Fix incorrect date handling in Phoenix 4 & 5 #10749

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions plugin/trino-phoenix/src/main/java/io/trino/plugin/phoenix/PhoenixClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import io.trino.plugin.jdbc.JdbcSplit;
import io.trino.plugin.jdbc.JdbcTableHandle;
import io.trino.plugin.jdbc.JdbcTypeHandle;
import io.trino.plugin.jdbc.LongReadFunction;
import io.trino.plugin.jdbc.LongWriteFunction;
import io.trino.plugin.jdbc.ObjectReadFunction;
import io.trino.plugin.jdbc.ObjectWriteFunction;
import io.trino.plugin.jdbc.PreparedQuery;
Expand Down Expand Up @@ -93,6 +95,9 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Types;
import java.text.SimpleDateFormat;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedList;
Expand All @@ -115,8 +120,6 @@
import static io.trino.plugin.jdbc.StandardColumnMappings.booleanColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.booleanWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.charWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.dateColumnMappingUsingSqlDate;
import static io.trino.plugin.jdbc.StandardColumnMappings.dateWriteFunctionUsingSqlDate;
import static io.trino.plugin.jdbc.StandardColumnMappings.decimalColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.defaultCharColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.defaultVarcharColumnMapping;
Expand Down Expand Up @@ -194,6 +197,9 @@ public class PhoenixClient
private static final String ROWKEY = "ROWKEY";
private static final long MAX_TOPN_LIMIT = 2000000;

private static final String DATE_FORMAT = "y-MM-dd G";
private static final DateTimeFormatter LOCAL_DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT);

private final Configuration configuration;

@Inject
Expand Down Expand Up @@ -428,7 +434,10 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
return Optional.of(varbinaryColumnMapping());

case Types.DATE:
return Optional.of(dateColumnMappingUsingSqlDate());
return Optional.of(ColumnMapping.longMapping(
DATE,
dateReadFunction(),
dateWriteFunctionUsingString()));

// TODO add support for TIMESTAMP after Phoenix adds support for LocalDateTime
case TIMESTAMP:
Expand Down Expand Up @@ -514,7 +523,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
}

if (type == DATE) {
return WriteMapping.longMapping("date", dateWriteFunctionUsingSqlDate());
return WriteMapping.longMapping("date", dateWriteFunctionUsingString());
}
if (TIME.equals(type)) {
return WriteMapping.longMapping("time", timeWriteFunctionUsingSqlTime());
Expand Down Expand Up @@ -695,6 +704,34 @@ public Map<String, Object> getTableProperties(ConnectorSession session, JdbcTabl
return properties.buildOrThrow();
}

private static LongReadFunction dateReadFunction()
{
return (resultSet, index) -> {
// Convert to LocalDate from java.sql.Date via String because java.sql.Date#toLocalDate() returns wrong results in B.C. dates. -5881579-07-11 -> +5881580-07-11
// Phoenix JDBC driver supports getObject(index, LocalDate.class), but it leads to incorrect issues. -5877641-06-23 -> 7642-06-23 & 5881580-07-11 -> 1580-07-11
// The current implementation still returns +10 days during julian -> gregorian switch
return LocalDate.parse(new SimpleDateFormat(DATE_FORMAT).format(resultSet.getDate(index)), LOCAL_DATE_FORMATTER).toEpochDay();
};
}

private static LongWriteFunction dateWriteFunctionUsingString()
{
return new LongWriteFunction() {
@Override
public String getBindExpression()
{
return "TO_DATE(?, 'y-MM-dd G', 'local')";
}

@Override
public void set(PreparedStatement statement, int index, long value)
throws SQLException
{
statement.setString(index, LOCAL_DATE_FORMATTER.format(LocalDate.ofEpochDay(value)));
}
};
}

private static ColumnMapping arrayColumnMapping(ConnectorSession session, ArrayType arrayType, String elementJdbcTypeName)
{
return ColumnMapping.objectMapping(
Expand Down
23 changes: 22 additions & 1 deletion plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,28 +493,49 @@ public void testDate(ZoneId sessionZone)
.setTimeZoneKey(getTimeZoneKey(sessionZone.getId()))
.build();

// TODO (https://github.com/trinodb/trino/issues/10074) Add more test cases when fixing incorrect date issue
SqlDataTypeTest.create()
.addRoundTrip("date", "DATE '-5877641-06-23'", DATE, "DATE '-5877641-06-23'") // min value in Trino
.addRoundTrip("date", "DATE '-0001-01-01'", DATE, "DATE '-0001-01-01'")
.addRoundTrip("date", "DATE '0001-01-01'", DATE, "DATE '0001-01-01'")
.addRoundTrip("date", "DATE '1582-10-04'", DATE, "DATE '1582-10-04'")
.addRoundTrip("date", "DATE '1582-10-05'", DATE, "DATE '1582-10-15'") // begin julian->gregorian switch
.addRoundTrip("date", "DATE '1582-10-14'", DATE, "DATE '1582-10-24'") // end julian->gregorian switch
.addRoundTrip("date", "DATE '1582-10-15'", DATE, "DATE '1582-10-15'")
.addRoundTrip("date", "DATE '1899-12-31'", DATE, "DATE '1899-12-31'")
.addRoundTrip("date", "DATE '1900-01-01'", DATE, "DATE '1900-01-01'")
.addRoundTrip("date", "DATE '1952-04-04'", DATE, "DATE '1952-04-04'") // before epoch
.addRoundTrip("date", "DATE '1970-01-01'", DATE, "DATE '1970-01-01'")
.addRoundTrip("date", "DATE '1970-02-03'", DATE, "DATE '1970-02-03'")
.addRoundTrip("date", "DATE '2017-07-01'", DATE, "DATE '2017-07-01'") // summer on northern hemisphere (possible DST)
.addRoundTrip("date", "DATE '2017-01-01'", DATE, "DATE '2017-01-01'") // winter on northern hemisphere (possible DST on southern hemisphere)
.addRoundTrip("date", "DATE '1983-04-01'", DATE, "DATE '1983-04-01'")
.addRoundTrip("date", "DATE '1983-10-01'", DATE, "DATE '1983-10-01'")
.addRoundTrip("date", "DATE '9999-12-31'", DATE, "DATE '9999-12-31'")
.addRoundTrip("date", "DATE '5881580-07-11'", DATE, "DATE '5881580-07-11'") // max value in Trino
.addRoundTrip("date", "NULL", DATE, "CAST(NULL AS DATE)")
.execute(getQueryRunner(), session, trinoCreateAsSelect(session, "test_date"))
.execute(getQueryRunner(), session, trinoCreateAsSelect(getSession(), "test_date"))
.execute(getQueryRunner(), session, trinoCreateAndInsert(session, "test_date"));

SqlDataTypeTest.create()
.addRoundTrip("date", "TO_DATE('5877642-06-23 BC', 'yyyy-MM-dd G', 'local')", DATE, "DATE '-5877641-06-23'") // min value in Trino
.addRoundTrip("date", "TO_DATE('0002-01-01 BC', 'yyyy-MM-dd G', 'local')", DATE, "DATE '-0001-01-01'")
.addRoundTrip("date", "TO_DATE('0001-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '0001-01-01'")
.addRoundTrip("date", "TO_DATE('1582-10-04', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-04'")
.addRoundTrip("date", "TO_DATE('1582-10-05', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-15'") // begin julian->gregorian switch
.addRoundTrip("date", "TO_DATE('1582-10-14', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-24'") // end julian->gregorian switch
.addRoundTrip("date", "TO_DATE('1582-10-15', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-15'")
.addRoundTrip("date", "TO_DATE('1899-12-31', 'yyyy-MM-dd', 'local')", DATE, "DATE '1899-12-31'")
.addRoundTrip("date", "TO_DATE('1900-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1900-01-01'")
.addRoundTrip("date", "TO_DATE('1952-04-04', 'yyyy-MM-dd', 'local')", DATE, "DATE '1952-04-04'") // before epoch
.addRoundTrip("date", "TO_DATE('1970-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1970-01-01'")
.addRoundTrip("date", "TO_DATE('1970-02-03', 'yyyy-MM-dd', 'local')", DATE, "DATE '1970-02-03'")
.addRoundTrip("date", "TO_DATE('2017-07-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '2017-07-01'") // summer on northern hemisphere (possible DST)
.addRoundTrip("date", "TO_DATE('2017-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '2017-01-01'") // winter on northern hemisphere (possible DST on southern hemisphere)
.addRoundTrip("date", "TO_DATE('1983-04-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1983-04-01'")
.addRoundTrip("date", "TO_DATE('1983-10-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1983-10-01'")
.addRoundTrip("date", "TO_DATE('9999-12-31', 'yyyy-MM-dd', 'local')", DATE, "DATE '9999-12-31'")
.addRoundTrip("date", "TO_DATE('5881580-07-11', 'yyyy-MM-dd', 'local')", DATE, "DATE '5881580-07-11'") // max value in Trino
.addRoundTrip("date", "NULL", DATE, "CAST(NULL AS DATE)")
.addRoundTrip("integer primary key", "1", INTEGER, "1")
.execute(getQueryRunner(), session, phoenixCreateAndInsert("tpch.test_date"));
Expand Down
45 changes: 41 additions & 4 deletions plugin/trino-phoenix5/src/main/java/io/trino/plugin/phoenix5/PhoenixClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@
import io.trino.plugin.jdbc.JdbcSplit;
import io.trino.plugin.jdbc.JdbcTableHandle;
import io.trino.plugin.jdbc.JdbcTypeHandle;
import io.trino.plugin.jdbc.LongReadFunction;
import io.trino.plugin.jdbc.LongWriteFunction;
import io.trino.plugin.jdbc.ObjectReadFunction;
import io.trino.plugin.jdbc.ObjectWriteFunction;
import io.trino.plugin.jdbc.PreparedQuery;
Expand Down Expand Up @@ -94,6 +96,9 @@
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Types;
import java.text.SimpleDateFormat;
import java.time.LocalDate;
import java.time.format.DateTimeFormatter;
import java.util.ArrayList;
import java.util.Collection;
import java.util.LinkedList;
Expand All @@ -116,8 +121,6 @@
import static io.trino.plugin.jdbc.StandardColumnMappings.booleanColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.booleanWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.charWriteFunction;
import static io.trino.plugin.jdbc.StandardColumnMappings.dateColumnMappingUsingSqlDate;
import static io.trino.plugin.jdbc.StandardColumnMappings.dateWriteFunctionUsingSqlDate;
import static io.trino.plugin.jdbc.StandardColumnMappings.decimalColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.defaultCharColumnMapping;
import static io.trino.plugin.jdbc.StandardColumnMappings.defaultVarcharColumnMapping;
Expand Down Expand Up @@ -194,6 +197,9 @@ public class PhoenixClient
{
private static final String ROWKEY = "ROWKEY";

private static final String DATE_FORMAT = "y-MM-dd G";
private static final DateTimeFormatter LOCAL_DATE_FORMATTER = DateTimeFormatter.ofPattern(DATE_FORMAT);

private final Configuration configuration;

@Inject
Expand Down Expand Up @@ -420,7 +426,10 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
return Optional.of(varbinaryColumnMapping());

case Types.DATE:
return Optional.of(dateColumnMappingUsingSqlDate());
return Optional.of(ColumnMapping.longMapping(
DATE,
dateReadFunction(),
dateWriteFunctionUsingString()));

// TODO add support for TIMESTAMP after Phoenix adds support for LocalDateTime
case TIMESTAMP:
Expand Down Expand Up @@ -506,7 +515,7 @@ public WriteMapping toWriteMapping(ConnectorSession session, Type type)
}

if (type == DATE) {
return WriteMapping.longMapping("date", dateWriteFunctionUsingSqlDate());
return WriteMapping.longMapping("date", dateWriteFunctionUsingString());
}
if (TIME.equals(type)) {
return WriteMapping.longMapping("time", timeWriteFunctionUsingSqlTime());
Expand Down Expand Up @@ -687,6 +696,34 @@ public Map<String, Object> getTableProperties(ConnectorSession session, JdbcTabl
return properties.buildOrThrow();
}

private static LongReadFunction dateReadFunction()
{
return (resultSet, index) -> {
// Convert to LocalDate from java.sql.Date via String because java.sql.Date#toLocalDate() returns wrong results in B.C. dates. -5881579-07-11 -> +5881580-07-11
// Phoenix JDBC driver supports getObject(index, LocalDate.class), but it leads to incorrect issues. -5877641-06-23 -> 7642-06-23 & 5881580-07-11 -> 1580-07-11
// The current implementation still returns +10 days during julian -> gregorian switch
return LocalDate.parse(new SimpleDateFormat(DATE_FORMAT).format(resultSet.getDate(index)), LOCAL_DATE_FORMATTER).toEpochDay();
};
}

private static LongWriteFunction dateWriteFunctionUsingString()
{
return new LongWriteFunction() {
@Override
public String getBindExpression()
{
return "TO_DATE(?, 'y-MM-dd G', 'local')";
}

@Override
public void set(PreparedStatement statement, int index, long value)
throws SQLException
{
statement.setString(index, LOCAL_DATE_FORMATTER.format(LocalDate.ofEpochDay(value)));
}
};
}

private static ColumnMapping arrayColumnMapping(ConnectorSession session, ArrayType arrayType, String elementJdbcTypeName)
{
return ColumnMapping.objectMapping(
Expand Down
23 changes: 22 additions & 1 deletion plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixTypeMapping.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,28 +493,49 @@ public void testDate(ZoneId sessionZone)
.setTimeZoneKey(getTimeZoneKey(sessionZone.getId()))
.build();

// TODO (https://github.com/trinodb/trino/issues/10074) Add more test cases when fixing incorrect date issue
SqlDataTypeTest.create()
.addRoundTrip("date", "DATE '-5877641-06-23'", DATE, "DATE '-5877641-06-23'") // min value in Trino
.addRoundTrip("date", "DATE '-0001-01-01'", DATE, "DATE '-0001-01-01'")
.addRoundTrip("date", "DATE '0001-01-01'", DATE, "DATE '0001-01-01'")
.addRoundTrip("date", "DATE '1582-10-04'", DATE, "DATE '1582-10-04'")
.addRoundTrip("date", "DATE '1582-10-05'", DATE, "DATE '1582-10-15'") // begin julian->gregorian switch
.addRoundTrip("date", "DATE '1582-10-14'", DATE, "DATE '1582-10-24'") // end julian->gregorian switch
.addRoundTrip("date", "DATE '1582-10-15'", DATE, "DATE '1582-10-15'")
.addRoundTrip("date", "DATE '1899-12-31'", DATE, "DATE '1899-12-31'")
.addRoundTrip("date", "DATE '1900-01-01'", DATE, "DATE '1900-01-01'")
.addRoundTrip("date", "DATE '1952-04-04'", DATE, "DATE '1952-04-04'") // before epoch
.addRoundTrip("date", "DATE '1970-01-01'", DATE, "DATE '1970-01-01'")
.addRoundTrip("date", "DATE '1970-02-03'", DATE, "DATE '1970-02-03'")
.addRoundTrip("date", "DATE '2017-07-01'", DATE, "DATE '2017-07-01'") // summer on northern hemisphere (possible DST)
.addRoundTrip("date", "DATE '2017-01-01'", DATE, "DATE '2017-01-01'") // winter on northern hemisphere (possible DST on southern hemisphere)
.addRoundTrip("date", "DATE '1983-04-01'", DATE, "DATE '1983-04-01'")
.addRoundTrip("date", "DATE '1983-10-01'", DATE, "DATE '1983-10-01'")
.addRoundTrip("date", "DATE '9999-12-31'", DATE, "DATE '9999-12-31'")
.addRoundTrip("date", "DATE '5881580-07-11'", DATE, "DATE '5881580-07-11'") // max value in Trino
.addRoundTrip("date", "NULL", DATE, "CAST(NULL AS DATE)")
.execute(getQueryRunner(), session, trinoCreateAsSelect(session, "test_date"))
.execute(getQueryRunner(), session, trinoCreateAsSelect(getSession(), "test_date"))
.execute(getQueryRunner(), session, trinoCreateAndInsert(session, "test_date"));

SqlDataTypeTest.create()
.addRoundTrip("date", "TO_DATE('5877642-06-23 BC', 'yyyy-MM-dd G', 'local')", DATE, "DATE '-5877641-06-23'") // min value in Trino
.addRoundTrip("date", "TO_DATE('0002-01-01 BC', 'yyyy-MM-dd G', 'local')", DATE, "DATE '-0001-01-01'")
.addRoundTrip("date", "TO_DATE('0001-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '0001-01-01'")
.addRoundTrip("date", "TO_DATE('1582-10-04', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-04'")
.addRoundTrip("date", "TO_DATE('1582-10-05', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-15'") // begin julian->gregorian switch
.addRoundTrip("date", "TO_DATE('1582-10-14', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-24'") // end julian->gregorian switch
.addRoundTrip("date", "TO_DATE('1582-10-15', 'yyyy-MM-dd', 'local')", DATE, "DATE '1582-10-15'")
.addRoundTrip("date", "TO_DATE('1899-12-31', 'yyyy-MM-dd', 'local')", DATE, "DATE '1899-12-31'")
.addRoundTrip("date", "TO_DATE('1900-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1900-01-01'")
.addRoundTrip("date", "TO_DATE('1952-04-04', 'yyyy-MM-dd', 'local')", DATE, "DATE '1952-04-04'") // before epoch
.addRoundTrip("date", "TO_DATE('1970-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1970-01-01'")
.addRoundTrip("date", "TO_DATE('1970-02-03', 'yyyy-MM-dd', 'local')", DATE, "DATE '1970-02-03'")
.addRoundTrip("date", "TO_DATE('2017-07-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '2017-07-01'") // summer on northern hemisphere (possible DST)
.addRoundTrip("date", "TO_DATE('2017-01-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '2017-01-01'") // winter on northern hemisphere (possible DST on southern hemisphere)
.addRoundTrip("date", "TO_DATE('1983-04-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1983-04-01'")
.addRoundTrip("date", "TO_DATE('1983-10-01', 'yyyy-MM-dd', 'local')", DATE, "DATE '1983-10-01'")
.addRoundTrip("date", "TO_DATE('9999-12-31', 'yyyy-MM-dd', 'local')", DATE, "DATE '9999-12-31'")
.addRoundTrip("date", "TO_DATE('5881580-07-11', 'yyyy-MM-dd', 'local')", DATE, "DATE '5881580-07-11'") // max value in Trino
.addRoundTrip("date", "NULL", DATE, "CAST(NULL AS DATE)")
.addRoundTrip("integer primary key", "1", INTEGER, "1")
.execute(getQueryRunner(), session, phoenixCreateAndInsert("tpch.test_date"));
Expand Down