Refactor AbstractTestQueries testcases for native-tests
#25486
Conversation
pramodsatya
requested review from
a team,
ClarenceThreepwood,
elharo,
feilong-liu and
jaystarshot
as code owners
prestodb-ci
requested review from
a team,
NivinCS and
nishithakbhaskaran
and removed request for
a team
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya for the refactoring.
| MaterializedResult actual2 = computeActual("SELECT approx_most_frequent(2, cast(x as bigint), 15) FROM (values 1, 2, 1, 3, 1, 2, 3, 4, 5) t(x)"); | ||
| assertEquals(actual2.getRowCount(), 1); | ||
| assertEquals(actual2.getMaterializedRows().get(0).getFields().get(0), ImmutableMap.of(1L, 3L, 2L, 2L)); | ||
| Object actual2Value = actual2.getMaterializedRows().get(0).getFields().get(0); |
There was a problem hiding this comment.
It seems like Java is always returning the first value for same number of duplicates while C++ picks the last one (and so you changed the results) ?. Can you check the behavior. If yes, then lets create an issue indicating the two sides resolve duplicates differently and document this behavior if need be.
There was a problem hiding this comment.
Yes that is the reason, although I would need to confirm whether C++ deterministically returns the last value when count is same. I feel it would be better to modify the values in the testcase instead of checking if the result matches either of the two expected rows so the result is not ambiguous; with something like this:
"SELECT approx_most_frequent(2, cast(x as bigint), 15) FROM (values 1, 2, 1, 3, 1, 2, 2, 1, 3, 4, 5) t(x)"
Would that be fine?
Update: Modified it as above so the output from approx_most_frequent is deterministic, could you PTAL?
| assertQueryFails( | ||
| "SELECT map_union_sum(x) from (select cast(MAP() as map<varchar, varchar>) x)", | ||
| ".*line 1:8: Unexpected parameters \\(map\\(varchar,varchar\\)\\) for function map_union_sum. Expected: map_union_sum\\(map\\(K,V\\)\\) K:comparable, V:nonDecimalNumeric.*"); | ||
| ".*Unexpected parameters \\(map\\(varchar,varchar\\)\\) for function map_union_sum.*"); |
There was a problem hiding this comment.
Does C++ not print out the expected signatures ? Actually its a good practice to print them out..its very useful for the user for debugging. Do you see different error messages with side-car vs not ? We could create an issue for Prestissimo for the error message.
There was a problem hiding this comment.
With the sidecar the error message is:
line 1:8: Unexpected parameters (map(varchar,varchar)) for function native.default.map_union_sum. Expected: native.default.map_union_sum(map(k,real)) k, native.default.map_union_sum(map(k,bigint)) k, native.default.map_union_sum(map(k,double)) k, native.default.map_union_sum(map(k,smallint)) k, native.default.map_union_sum(map(k,tinyint)) k, native.default.map_union_sum(map(k,integer)) k
I had removed string for Expected function signatures because the substring K:comparable, V:nonDecimalNumeric is missing in the Presto C++ error message. Also the error message with Presto C++ expected signatures would be different from the Presto expected signatures so not sure if we need to match them exactly?
I could create an issue for any missing map_union_sum function signatures in Velox. Would that be fine?
There was a problem hiding this comment.
Thanks for pointing it out, modified the error message and opened a fix in Velox here: facebookincubator/velox#14049. Could you please take another look?
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
Outdated
Show resolved
Hide resolved
presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestQueries.java
Outdated
Show resolved
Hide resolved
|
|
||
| private ZonedDateTime zonedDateTime(String value) | ||
| @Test | ||
| public void testAtTimeZoneWithInterval() |
There was a problem hiding this comment.
Why did you separate this test ?
There was a problem hiding this comment.
Velox does not support at_timezone function with Interval day time as second argument, and this is the only query from testAtTimeZone testing for this function signature. I will create an issue and link it in the overriden testcase in the main PR.
pramodsatya
force-pushed
the
refactor_tests
branch
from
790d87e to
a33f89f
Compare
|
Thanks for the feedback @aditi-pandit, addressed the review comments. Could you please take another look? |
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
Thanks @pramodsatya. Current changes look good.
|
@tdcmeehan : Please can you help with the review. This is helpful for the last PR for native tests. |
4 participants
Description
Refactor testcases in
AbstractTestQueriesfrompresto-testsforpresto-native-teststo be added in #23671.