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

SQL: be lenient for tests involving comparison to H2 but strict for csv spec tests #36498

Merged
merged 5 commits into from
Dec 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLEx
// uncomment this to printout the result set and create new CSV tests
//
//JdbcTestUtils.logLikeCLI(elastic, log);
JdbcAssert.assertResultSets(expected, elastic, log, true);
JdbcAssert.assertResultSets(expected, elastic, log, true, false);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;

import org.apache.logging.log4j.Logger;
import org.elasticsearch.xpack.sql.qa.jdbc.CsvTestUtils.CsvTestCase;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;

Expand Down Expand Up @@ -58,4 +60,10 @@ protected final void doTest() throws Throwable {
assertResults(expected, elasticResults);
}
}

@Override
protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLException {
Logger log = logEsResultSet() ? logger : null;
JdbcAssert.assertResultSets(expected, elastic, log, false, false);
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
/*
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
Expand Down Expand Up @@ -57,15 +58,29 @@ public static void assertResultSets(ResultSet expected, ResultSet actual, Logger

/**
* Assert the given result sets, potentially in a lenient way.
* When lenient is specified, the type comparison of a column is widden to reach a common, compatible ground.
* When lenientDataType is specified, the type comparison of a column is widden to reach a common, compatible ground.
* This means promoting integer types to long and floating types to double and comparing their values.
* For example in a non-lenient, strict case a comparison between an int and a tinyint would fail, with lenient it will succeed as
* long as the actual value is the same.
* For example in a non-lenient, strict case a comparison between an int and a tinyint would fail, with lenientDataType it will succeed
* as long as the actual value is the same.
*/
public static void assertResultSets(ResultSet expected, ResultSet actual, Logger logger, boolean lenient) throws SQLException {
public static void assertResultSets(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType) throws SQLException {
assertResultSets(expected, actual, logger, lenientDataType, true);
}

/**
* Assert the given result sets, potentially in a lenient way.
* When lenientDataType is specified, the type comparison of a column is widden to reach a common, compatible ground.
* This means promoting integer types to long and floating types to double and comparing their values.
* For example in a non-lenient, strict case a comparison between an int and a tinyint would fail, with lenientDataType it will succeed
* as long as the actual value is the same.
* Also, has the option of treating the numeric results for floating point numbers in a leninent way, if chosen to. Usually,
* we would want lenient treatment for floating point numbers in sql-spec tests where the comparison is being made with H2.
*/
public static void assertResultSets(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType,
Copy link
Member

Choose a reason for hiding this comment

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

Keep the previous method with reduced parameters and delegate to the new one with a default value. This would minimize the amount of changes since the leniency needs to be disabled/enabled just in some cases .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @costin. I agree with the fact that there will be more changes to test methods, but this approach is intentional, so that the user of this method becomes aware of the leniency regarding floating point numbers, something that wasn't obvious before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

boolean lenientFloatingNumbers) throws SQLException {
try (ResultSet ex = expected; ResultSet ac = actual) {
assertResultSetMetadata(ex, ac, logger, lenient);
assertResultSetData(ex, ac, logger, lenient);
assertResultSetMetadata(ex, ac, logger, lenientDataType);
assertResultSetData(ex, ac, logger, lenientDataType, lenientFloatingNumbers);
}
}

Expand All @@ -74,7 +89,8 @@ public static void assertResultSetMetadata(ResultSet expected, ResultSet actual,
}

// metadata doesn't consume a ResultSet thus it shouldn't close it
public static void assertResultSetMetadata(ResultSet expected, ResultSet actual, Logger logger, boolean lenient) throws SQLException {
public static void assertResultSetMetadata(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType)
throws SQLException {
ResultSetMetaData expectedMeta = expected.getMetaData();
ResultSetMetaData actualMeta = actual.getMetaData();

Expand Down Expand Up @@ -116,8 +132,8 @@ public static void assertResultSetMetadata(ResultSet expected, ResultSet actual,
}

// use the type not the name (timestamp with timezone returns spaces for example)
int expectedType = typeOf(expectedMeta.getColumnType(column), lenient);
int actualType = typeOf(actualMeta.getColumnType(column), lenient);
int expectedType = typeOf(expectedMeta.getColumnType(column), lenientDataType);
int actualType = typeOf(actualMeta.getColumnType(column), lenientDataType);

// since H2 cannot use a fixed timezone, the data is stored in UTC (and thus with timezone)
if (expectedType == Types.TIMESTAMP_WITH_TIMEZONE) {
Expand Down Expand Up @@ -153,13 +169,20 @@ public static void assertResultSetData(ResultSet expected, ResultSet actual, Log
assertResultSetData(expected, actual, logger, false);
}

public static void assertResultSetData(ResultSet expected, ResultSet actual, Logger logger, boolean lenient) throws SQLException {
public static void assertResultSetData(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType)
throws SQLException {
assertResultSetData(expected, actual, logger, lenientDataType, true);
}

public static void assertResultSetData(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType,
boolean lenientFloatingNumbers) throws SQLException {
try (ResultSet ex = expected; ResultSet ac = actual) {
doAssertResultSetData(ex, ac, logger, lenient);
doAssertResultSetData(ex, ac, logger, lenientDataType, lenientFloatingNumbers);
}
}

private static void doAssertResultSetData(ResultSet expected, ResultSet actual, Logger logger, boolean lenient) throws SQLException {
private static void doAssertResultSetData(ResultSet expected, ResultSet actual, Logger logger, boolean lenientDataType,
boolean lenientFloatingNumbers) throws SQLException {
ResultSetMetaData metaData = expected.getMetaData();
int columns = metaData.getColumnCount();

Expand Down Expand Up @@ -199,7 +222,7 @@ private static void doAssertResultSetData(ResultSet expected, ResultSet actual,
}

Object expectedObject = expected.getObject(column);
Object actualObject = lenient ? actual.getObject(column, expectedColumnClass) : actual.getObject(column);
Object actualObject = lenientDataType ? actual.getObject(column, expectedColumnClass) : actual.getObject(column);

String msg = format(Locale.ROOT, "Different result for column [%s], entry [%d]",
metaData.getColumnName(column), count + 1);
Expand All @@ -219,10 +242,9 @@ else if (type == Types.TIMESTAMP || type == Types.TIMESTAMP_WITH_TIMEZONE) {
}
// and floats/doubles
else if (type == Types.DOUBLE) {
// the 1d/1f difference is used due to rounding/flooring
assertEquals(msg, (double) expectedObject, (double) actualObject, 1d);
assertEquals(msg, (double) expectedObject, (double) actualObject, lenientFloatingNumbers ? 1d : 0.0d);
} else if (type == Types.FLOAT) {
assertEquals(msg, (float) expectedObject, (float) actualObject, 1f);
assertEquals(msg, (float) expectedObject, (float) actualObject, lenientFloatingNumbers ? 1f : 0.0f);
}
// intervals
else if (type == Types.VARCHAR && actualObject instanceof TemporalAmount) {
Expand Down Expand Up @@ -251,8 +273,8 @@ else if (type == Types.VARCHAR && actualObject instanceof TemporalAmount) {
/**
* Returns the value of the given type either in a lenient fashion (widened) or strict.
*/
private static int typeOf(int columnType, boolean lenient) {
if (lenient) {
private static int typeOf(int columnType, boolean lenientDataType) {
if (lenientDataType) {
// integer upcast to long
if (columnType == TINYINT || columnType == SMALLINT || columnType == INTEGER || columnType == BIGINT) {
return BIGINT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -197,4 +197,4 @@ public interface Parser {
public static InputStream readFromJarUrl(URL source) throws IOException {
return source.openStream();
}
}
}
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/qa/src/main/resources/agg.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ SELECT gender, PERCENTILE(emp_no, 97.76) p1 FROM test_emp GROUP BY gender;

gender:s | p1:d
null |10019.0
F |10099.51
M |10095.789999999999
F |10099.7608
M |10096.2232
;

multiplePercentilesOneWithCommaOneWithout
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/qa/src/main/resources/alias.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,6 @@ SELECT gender, PERCENTILE(emp_no, 97) p1 FROM "test_*" WHERE gender is NOT NULL

gender:s | p1:d

F | 10099.32
M | 10095.98
F | 10099.52
M | 10096.0
;
16 changes: 8 additions & 8 deletions x-pack/plugin/sql/qa/src/main/resources/docs.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ SELECT MIN(salary) AS min, MAX(salary) AS max, AVG(salary) AS avg, COUNT(*) AS c

min:i | max:i | avg:d | count:l
---------------+---------------+---------------+---------------
25324 |74999 |48248 |100
25324 |74999 |48248.55 |100

// end::groupByImplicitMultipleAggs
;
Expand Down Expand Up @@ -724,9 +724,9 @@ SELECT SCORE(), * FROM library WHERE MATCH(name, 'dune') ORDER BY SCORE() DESC;

SCORE() | author | name | page_count | release_date
---------------+---------------+-------------------+---------------+--------------------
2.288635 |Frank Herbert |Dune |604 |1965-06-01T00:00:00Z
2.2886353 |Frank Herbert |Dune |604 |1965-06-01T00:00:00Z
1.8893257 |Frank Herbert |Dune Messiah |331 |1969-10-15T00:00:00Z
1.6086555 |Frank Herbert |Children of Dune |408 |1976-04-21T00:00:00Z
1.6086556 |Frank Herbert |Children of Dune |408 |1976-04-21T00:00:00Z
1.4005898 |Frank Herbert |God Emperor of Dune|454 |1981-05-28T00:00:00Z

// end::orderByScore
Expand All @@ -738,9 +738,9 @@ SELECT SCORE(), * FROM library WHERE MATCH(name, 'dune') ORDER BY page_count DES

SCORE() | author | name | page_count | release_date
---------------+---------------+-------------------+---------------+--------------------
2.288635 |Frank Herbert |Dune |604 |1965-06-01T00:00:00Z
2.2886353 |Frank Herbert |Dune |604 |1965-06-01T00:00:00Z
1.4005898 |Frank Herbert |God Emperor of Dune|454 |1981-05-28T00:00:00Z
1.6086555 |Frank Herbert |Children of Dune |408 |1976-04-21T00:00:00Z
1.6086556 |Frank Herbert |Children of Dune |408 |1976-04-21T00:00:00Z
1.8893257 |Frank Herbert |Dune Messiah |331 |1969-10-15T00:00:00Z

// end::orderByScoreWithMatch
Expand All @@ -753,9 +753,9 @@ SELECT SCORE() AS score, name, release_date FROM library WHERE QUERY('dune') ORD
score | name | release_date
---------------+-------------------+--------------------
1.4005898 |God Emperor of Dune|1981-05-28T00:00:00Z
1.6086555 |Children of Dune |1976-04-21T00:00:00Z
1.6086556 |Children of Dune |1976-04-21T00:00:00Z
1.8893257 |Dune Messiah |1969-10-15T00:00:00Z
2.288635 |Dune |1965-06-01T00:00:00Z
2.2886353 |Dune |1965-06-01T00:00:00Z
// end::scoreWithMatch
;

Expand Down Expand Up @@ -789,7 +789,7 @@ SELECT AVG(salary) AS avg FROM emp;

avg:d
---------------
48248
48248.55
// end::aggAvg
;

Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugin/sql/qa/src/main/resources/fulltext.csv-spec
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,12 @@ score
SELECT emp_no, first_name, SCORE() FROM test_emp WHERE MATCH(first_name, 'Erez') ORDER BY SCORE();

emp_no:i | first_name:s | SCORE():f
10076 |Erez |4.2096553
10076 |Erez |4.1053944
;

scoreAsSomething
SELECT emp_no, first_name, SCORE() as s FROM test_emp WHERE MATCH(first_name, 'Erez') ORDER BY SCORE();

emp_no:i | first_name:s | s:f
10076 |Erez |4.2096553
10076 |Erez |4.1053944
;