DRILL-7237: Fix single_value aggregate function for variable length types #1782
Conversation
vvysotskyi
commented
May 6, 2019
vvysotskyi
commented
- Added implementations of single_value for complex data types
|
@KazydubB could you please review? |
| result.release(); | ||
| for (String columnName : columns) { | ||
| try { | ||
| test("select single_value(t.%1$s) as %1$s from %2$s t", columnName, tableName); |
There was a problem hiding this comment.
Why not use thrown field instead of try-catch as is done in method below (testSingleValueWithMultipleComplexInputs)?
There was a problem hiding this comment.
It was done because after the first query is failed, thrown checks the error, but the loop is not continued.
exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestAggregateFunctions.java
Outdated
Show resolved
Hide resolved
| "cp.`parquet/alltypes_required.parquet`", | ||
| "cp.`parquet/alltypes_optional.parquet`"); | ||
| for (String tableName : tableNames) { | ||
| QueryDataBatch result = |
There was a problem hiding this comment.
It'd be good to move logic from inner for-loops (here and below) to separate private methods, so that the method's body is not that large, what do you think?
There was a problem hiding this comment.
Agree, moved the logic for constructing baseline values into a separate method, but left code with a query in the test method for better readability.
…ypes - Add implementations of single_value for complex data types
There was a problem hiding this comment.
@KazydubB, thanks for the review, I have addressed your comments. Could you please take a look again?
| "cp.`parquet/alltypes_required.parquet`", | ||
| "cp.`parquet/alltypes_optional.parquet`"); | ||
| for (String tableName : tableNames) { | ||
| QueryDataBatch result = |
There was a problem hiding this comment.
Agree, moved the logic for constructing baseline values into a separate method, but left code with a query in the test method for better readability.
| result.release(); | ||
| for (String columnName : columns) { | ||
| try { | ||
| test("select single_value(t.%1$s) as %1$s from %2$s t", columnName, tableName); |
There was a problem hiding this comment.
It was done because after the first query is failed, thrown checks the error, but the loop is not continued.
|
@vvysotskyi thanks, LGTM +1. |
|
@arina-ielchiieva - Bohdan is not a committer, so can you please give a +1 ? |
|
+1 By the way, @Ben-Zvi as batch committer you don't need other committer +1 to commit the changes unless you are completely unsure what is going on in the PR. |
|
Thanks @arina-ielchiieva ; in such a case a committer's +1 means - "I trust the reviewer's specific knowledge to do this review properly". And since you are more familiar with Bohdan's knowledge, I asked for your +1. -- Thanks. |
…ypes - Add implementations of single_value for complex data types closes apache#1782
…ypes - Add implementations of single_value for complex data types closes apache#1782 (cherry picked from commit 0195d1f)
…ypes - Add implementations of single_value for complex data types closes apache#1782
