Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19660 )
Change subject: IMPALA-12019: Support ORDER BY for arrays of fixed length types in select list ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/19660/7/be/src/runtime/collection-value.h File be/src/runtime/collection-value.h: http://gerrit.cloudera.org:8080/#/c/19660/7/be/src/runtime/collection-value.h@60 PS7, Line 60: int64_t byte_size; int32_t should be enough to store this, as the whole row should fit to that size AFAIK I don't mind using int64_t, but some comment could be added about the expected byte sizes http://gerrit.cloudera.org:8080/#/c/19660/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java File fe/src/main/java/org/apache/impala/analysis/QueryStmt.java: http://gerrit.cloudera.org:8080/#/c/19660/7/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@407 PS7, Line 407: // TODO: Should we include datetime? : return itemType.isBoolean() || : itemType.isScalarType(PrimitiveType.TINYINT) || : itemType.isScalarType(PrimitiveType.SMALLINT) || : itemType.isScalarType(PrimitiveType.INT) || : itemType.isScalarType(PrimitiveType.BIGINT) || : itemType.isScalarType(PrimitiveType.FLOAT) || : itemType.isScalarType(PrimitiveType.DOUBLE) || : itemType.isDecimal() || : itemType.isTimestamp() || : itemType.isDate(); could be clearer and shorter with !itemType.isStringType() && !itemType.isStringType().isComplexType() http://gerrit.cloudera.org:8080/#/c/19660/7/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test File testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test: http://gerrit.cloudera.org:8080/#/c/19660/7/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@38 PS7, Line 38: # Sort a collection. About the location of the tests: adding them to sort.test could ensure that they are run with a wider set of sorting related options. http://gerrit.cloudera.org:8080/#/c/19660/7/testdata/workloads/functional-query/queries/QueryTest/nested-array-in-select-list.test@39 PS7, Line 39: Does the patch also support ordey by limit N? Can you also add some tests for it, even if it is not supported? -- To view, visit http://gerrit.cloudera.org:8080/19660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7974ef392c1412e8c60231e3420367bd189677a Gerrit-Change-Number: 19660 Gerrit-PatchSet: 7 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Comment-Date: Mon, 08 May 2023 11:00:44 +0000 Gerrit-HasComments: Yes
