Matthew Jacobs has posted comments on this change. Change subject: IMPALA-3931: arbitrary fixed-size agg intermediate types ......................................................................
Patch Set 6: (5 comments) Looks good to me, though I'm leaving the codegen stuff for Michael to review http://gerrit.cloudera.org:8080/#/c/7526/6//COMMIT_MSG Commit Message: PS6, Line 26: FIXED_AGG_INTERMEDIATE I think FIXED_UDA_INTERMEDIATE would be more precise/clear, especially since this applies to the AnalyticEvalNode as well. That said, I realize it'd be annoying to change everywhere. PS6, Line 37: Testing: any corner cases to consider for analytic fns? I think it should work fine, though it occurred to me we don't have many tests where analytic fns spill. I think there are a couple in analytic-fns.test but those aren't using fns that will make use of the new type. http://gerrit.cloudera.org:8080/#/c/7526/6/be/src/runtime/types.cc File be/src/runtime/types.cc: PS6, Line 172: TypeToOdbcString should types that aren't exposed raise a dcheck? PS6, Line 302: // HiveServer2 does not have a type for invalid, date, datetime or : // fixed_agg_intermediate. : DCHECK(false) << "bad TypeToTValueType() type: " << DebugString(); : type_entry.__set_type(TTypeId::STRING_TYPE); something similar for odbc seems applicable http://gerrit.cloudera.org:8080/#/c/7526/6/fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java File fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java: PS6, Line 1000: TODO ? rankIntermediateType should work -- To view, visit http://gerrit.cloudera.org:8080/7526 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ife90cf27989f98ffb5ef5c39f1e09ce92e8cb87c Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho Gerrit-HasComments: Yes
