SQL: be lenient for tests involving comparison to H2 but strict for csv spec tests #36498
Conversation
tests as not lenient vs. sql-spec tests to be lenient.
|
Pinging @elastic/es-search |
|
run the gradle build tests 2 |
| * 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 lening treatment for floating point numbers in sql-spec tests where the comparison is being made with H2. |
There was a problem hiding this comment.
Yes. Should be lenient. Thanks.
| protected void assertResults(ResultSet expected, ResultSet elastic) throws SQLException { | ||
| /* | ||
| * Asserts the numeric results for floating point numbers in a leninent way, if chosen to. Usually, | ||
| * we would want lening treatment for floating point numbers in sql-spec tests where the comparison is being made with H2. |
There was a problem hiding this comment.
It should be lenient :-). Will change.
| */ | ||
| 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, |
There was a problem hiding this comment.
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 .
There was a problem hiding this comment.
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.
| @@ -31,7 +31,7 @@ public void testGetProcedures() throws Exception { | |||
| assertResultSets(expected, es.getMetaData().getProcedures( | |||
| randomBoolean() ? null : randomAlphaOfLength(5), | |||
| randomBoolean() ? null : randomAlphaOfLength(5), | |||
| randomBoolean() ? null : randomAlphaOfLength(5))); | |||
| randomBoolean() ? null : randomAlphaOfLength(5)), true); | |||
There was a problem hiding this comment.
See the comment below - the short assertResultSets signature should be kept so then test would only have to specify when they don't want to be lenient as oppose to all the time.
…sv spec tests (elastic#36498) * Add a lenient parameter for floating point numbers and mark csv-spec tests as not lenient vs. sql-spec tests to be lenient. (cherry picked from commit 1270d85)
This PR addresses #36483 by adding a new boolean lenient parameter for tests to round or not the floating point numbers in tests. Relates to https://github.com/elastic/elasticsearch/pull/36431/files#r240219710.