From e8ab7ca0978fd45f825b4b6215869edb86ec4e9a Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 3 Apr 2019 13:22:59 +0200 Subject: [PATCH 1/2] SQL: Fix deserialization issue of TimeProcessor TimeProcessor didn't implement `getWriteableName()` so the one from the parent was used which returned the `NAME` of the parent. This caused `TimeProcessor` objects to be deserialised into DateTimeProcessor. Moreover, added a restriction to run the TIME related integration tests only in UTC timezone. Fixes: #40717 --- .../xpack/sql/jdbc/TypeConverter.java | 3 +- .../xpack/sql/qa/jdbc/CsvSpecTestCase.java | 21 +++++-- .../sql/qa/jdbc/JdbcIntegrationTestCase.java | 3 +- .../qa/jdbc/SpecBaseIntegrationTestCase.java | 5 +- .../sql/qa/src/main/resources/time.csv-spec | 55 +++++++++---------- .../scalar/datetime/TimeProcessor.java | 6 +- .../xpack/sql/expression/ProcessorTests.java | 46 ++++++++-------- 7 files changed, 77 insertions(+), 62 deletions(-) diff --git a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java index 35f8241fb4786..9c30241ccbdb1 100644 --- a/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java +++ b/x-pack/plugin/sql/jdbc/src/main/java/org/elasticsearch/xpack/sql/jdbc/TypeConverter.java @@ -39,6 +39,7 @@ import static org.elasticsearch.xpack.sql.jdbc.EsType.DATETIME; import static org.elasticsearch.xpack.sql.jdbc.EsType.TIME; import static org.elasticsearch.xpack.sql.jdbc.JdbcDateUtils.asDateTimeField; +import static org.elasticsearch.xpack.sql.jdbc.JdbcDateUtils.timeAsTime; /** * Conversion utilities for conversion of JDBC types to Java type and back @@ -220,7 +221,7 @@ static Object convert(Object v, EsType columnType, String typeString) throws SQL case DATE: return asDateTimeField(v, JdbcDateUtils::asDate, Date::new); case TIME: - return asDateTimeField(v, JdbcDateUtils::asTime, Time::new); + return timeAsTime(v.toString()); case DATETIME: return asDateTimeField(v, JdbcDateUtils::asTimestamp, Timestamp::new); case INTERVAL_YEAR: diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java index 7f5077f15a885..56402e028cae2 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java @@ -40,12 +40,14 @@ public CsvSpecTestCase(String fileName, String groupName, String testName, Integ @Override protected final void doTest() throws Throwable { - try (Connection csv = csvConnection(testCase); Connection es = esJdbc()) { - - // pass the testName as table for debugging purposes (in case the underlying reader is missing) - ResultSet expected = executeCsvQuery(csv, testName); - ResultSet elasticResults = executeJdbcQuery(es, testCase.query); - assertResults(expected, elasticResults); + if ("time".equals(groupName)) { // Run the time tests always in UTC + try (Connection csv = csvConnection(testCase); Connection es = esJdbc(connectionProperties())) { + executeAndAssert(csv, es); + } + } else { + try (Connection csv = csvConnection(testCase); Connection es = esJdbc()) { + executeAndAssert(csv, es); + } } } @@ -54,4 +56,11 @@ protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLEx Logger log = logEsResultSet() ? logger : null; JdbcAssert.assertResultSets(expected, elastic, log, false, true); } + + private void executeAndAssert(Connection csv, Connection es) throws SQLException { + // pass the testName as table for debugging purposes (in case the underlying reader is missing) + ResultSet expected = executeCsvQuery(csv, testName); + ResultSet elasticResults = executeJdbcQuery(es, testCase.query); + assertResults(expected, elasticResults); + } } diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcIntegrationTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcIntegrationTestCase.java index 5a5ac373cd4e4..4d07815b49fbc 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcIntegrationTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/JdbcIntegrationTestCase.java @@ -27,6 +27,7 @@ import java.util.Properties; import java.util.Set; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.JDBC_TIMEZONE; import static org.elasticsearch.xpack.sql.qa.rest.RestSqlTestCase.assertNoSearchContexts; public abstract class JdbcIntegrationTestCase extends ESRestTestCase { @@ -137,7 +138,7 @@ protected String clusterName() { */ protected Properties connectionProperties() { Properties connectionProperties = new Properties(); - connectionProperties.put("timezone", randomKnownTimeZone()); + connectionProperties.put(JDBC_TIMEZONE, randomKnownTimeZone()); // in the tests, don't be lenient towards multi values connectionProperties.put("field.multi.value.leniency", "false"); return connectionProperties; diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SpecBaseIntegrationTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SpecBaseIntegrationTestCase.java index eaca892ea43b2..4282b97d87fb1 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SpecBaseIntegrationTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/SpecBaseIntegrationTestCase.java @@ -32,6 +32,7 @@ import java.util.Properties; import static java.util.Collections.emptyList; +import static org.elasticsearch.xpack.sql.qa.jdbc.JdbcTestUtils.JDBC_TIMEZONE; /** * Tests that compare the Elasticsearch JDBC client to some other JDBC client @@ -116,7 +117,7 @@ protected int fetchSize() { @Override protected Properties connectionProperties() { Properties connectionProperties = new Properties(); - connectionProperties.setProperty("timezone", "UTC"); + connectionProperties.setProperty(JDBC_TIMEZONE, "UTC"); return connectionProperties; } @@ -217,4 +218,4 @@ public interface Parser { public static InputStream readFromJarUrl(URL source) throws IOException { return source.openStream(); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec index cba8dd29a75a1..6545755e7588a 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec @@ -1,26 +1,26 @@ // // TIME // +// All tests are run with UTC timezone -// AwaitsFix: https://github.com/elastic/elasticsearch/issues/40717 -//timeExtractTimeParts -//SELECT -//SECOND(CAST(birth_date AS TIME)) d, -//MINUTE(CAST(birth_date AS TIME)) m, -//HOUR(CAST(birth_date AS TIME)) h -//FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no; -// -// d:i | m:i | h:i -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//0 |0 |0 -//; +timeExtractTimeParts +SELECT +SECOND(CAST(birth_date AS TIME)) d, +MINUTE(CAST(birth_date AS TIME)) m, +HOUR(CAST(birth_date AS TIME)) h +FROM "test_emp" WHERE emp_no < 10010 ORDER BY emp_no; + + d:i | m:i | h:i +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +0 |0 |0 +; timeAsFilter SELECT birth_date, last_name FROM "test_emp" WHERE birth_date::TIME = CAST('00:00:00' AS TIME) ORDER BY emp_no LIMIT 5; @@ -59,15 +59,14 @@ null |100445 0 |904605 ; -// AwaitsFix: https://github.com/elastic/elasticsearch/issues/40717 -//timeAsHavingFilter -//SELECT MINUTE_OF_HOUR(MAX(birth_date)::TIME + INTERVAL 10 MINUTES) as minute, gender FROM test_emp GROUP BY gender HAVING CAST(MAX(birth_date) AS TIME) = CAST('00:00:00.000' AS TIME) ORDER BY gender; -// -//minute:i | gender:s -//10 | null -//10 | F -//10 | M -//; +timeAsHavingFilter +SELECT MINUTE_OF_HOUR(MAX(birth_date)::TIME + INTERVAL 10 MINUTES) as minute, gender FROM test_emp GROUP BY gender HAVING CAST(MAX(birth_date) AS TIME) = CAST('00:00:00.000' AS TIME) ORDER BY gender; + +minute:i | gender:s +10 | null +10 | F +10 | M +; timeAsHavingFilterNoMatch SELECT MINUTE_OF_HOUR(MAX(birth_date)::TIME) as minute, gender FROM test_emp GROUP BY gender HAVING CAST(MAX(birth_date) AS TIME) > CAST('00:00:00.000' AS TIME); diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java index a263d83dcb195..e0b221aea7328 100644 --- a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java +++ b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/expression/function/scalar/datetime/TimeProcessor.java @@ -16,7 +16,6 @@ public class TimeProcessor extends DateTimeProcessor { - public static final String NAME = "time"; public TimeProcessor(DateTimeExtractor extractor, ZoneId zoneId) { @@ -27,6 +26,11 @@ public TimeProcessor(StreamInput in) throws IOException { super(in); } + @Override + public String getWriteableName() { + return NAME; + } + @Override public Object process(Object input) { if (input instanceof OffsetTime) { diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/ProcessorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/ProcessorTests.java index e3d51f1f7dd87..a01178fbc5b79 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/ProcessorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/expression/ProcessorTests.java @@ -30,7 +30,6 @@ public static void init() throws Exception { processors = NodeSubclassTests.subclassesOf(Processor.class); } - public void testProcessorRegistration() throws Exception { LinkedHashSet registered = Processors.getNamedWriteables().stream() .map(e -> e.name) @@ -39,32 +38,33 @@ public void testProcessorRegistration() throws Exception { // discover available processors int missing = processors.size() - registered.size(); + List notRegistered = new ArrayList<>(); + for (Class proc : processors) { + String procName = proc.getName(); + assertTrue(procName + " does NOT implement NamedWriteable", NamedWriteable.class.isAssignableFrom(proc)); + Field name = null; + String value = null; + try { + name = proc.getField("NAME"); + } catch (Exception ex) { + fail(procName + " does NOT provide a NAME field\n" + ex); + } + try { + value = name.get(proc).toString(); + } catch (Exception ex) { + fail(procName + " does NOT provide a static NAME field\n" + ex); + } + if (!registered.contains(value)) { + notRegistered.add(procName); + } + Class declaringClass = proc.getMethod("getWriteableName").getDeclaringClass(); + assertEquals("Processor: " + proc + " doesn't override getWriteableName", proc, declaringClass); + } if (missing > 0) { - List notRegistered = new ArrayList<>(); - for (Class proc : processors) { - String procName = proc.getName(); - assertTrue(procName + " does NOT implement NamedWriteable", NamedWriteable.class.isAssignableFrom(proc)); - Field name = null; - String value = null; - try { - name = proc.getField("NAME"); - } catch (Exception ex) { - fail(procName + " does NOT provide a NAME field\n" + ex); - } - try { - value = name.get(proc).toString(); - } catch (Exception ex) { - fail(procName + " does NOT provide a static NAME field\n" + ex); - } - if (!registered.contains(value)) { - notRegistered.add(procName); - } - } - fail(missing + " processor(s) not registered : " + notRegistered); } else { assertEquals("Detection failed: discovered more registered processors than classes", 0, missing); } } -} \ No newline at end of file +} From 6c53bd40a354e2a250934462d8717a3057cd6d26 Mon Sep 17 00:00:00 2001 From: Marios Trivyzas Date: Wed, 3 Apr 2019 14:54:16 +0200 Subject: [PATCH 2/2] Address comment --- .../org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java | 4 +++- x-pack/plugin/sql/qa/src/main/resources/time.csv-spec | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java index 56402e028cae2..40ce2416c249c 100644 --- a/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java +++ b/x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/CsvSpecTestCase.java @@ -40,7 +40,9 @@ public CsvSpecTestCase(String fileName, String groupName, String testName, Integ @Override protected final void doTest() throws Throwable { - if ("time".equals(groupName)) { // Run the time tests always in UTC + // Run the time tests always in UTC + // TODO: https://github.com/elastic/elasticsearch/issues/40779 + if ("time".equals(groupName)) { try (Connection csv = csvConnection(testCase); Connection es = esJdbc(connectionProperties())) { executeAndAssert(csv, es); } diff --git a/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec b/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec index 6545755e7588a..d599497fc2255 100644 --- a/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec +++ b/x-pack/plugin/sql/qa/src/main/resources/time.csv-spec @@ -2,6 +2,7 @@ // TIME // // All tests are run with UTC timezone +// TODO: https://github.com/elastic/elasticsearch/issues/40779 timeExtractTimeParts SELECT