You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org> on 2023/05/08 11:00:44 UTC
[Impala-ASF-CR] IMPALA-12019: Support ORDER BY for arrays of fixed length types in select list
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 <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Noemi Pap-Takacs <np...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 May 2023 11:00:44 +0000
Gerrit-HasComments: Yes