Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/21643 )
Change subject: IMPALA-13272: Analyitic function of collections can lead to crash ...................................................................... Patch Set 2: (5 comments) Thanks for the quick fix! http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java File fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java: http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@154 PS2, Line 154: materialised nit: we usually write it with a 'z'. Even if both forms are correct, it makes sense to write only one form, "materialized", so it is easier to search for it in the code base. Please note that it is not the only occurence of 'materialised' in this commit message. http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@161 PS2, Line 161: isMaterializedRecursively It's a bit confusing to have isMaterializedRecursively() and isFullyMaterialized() with different meanings. Maybe we should rename the former to 'ShouldComputeLayoutRecursively()' or something like that. http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java File fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java: http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@332 PS2, Line 332: materialised nit: as I mentioned, IMO it is not good to mix 'materialised' and 'materialized'. In code we are using 'materialize', so I think we should stick to that. 'Materialize' is also more common in the SQL literature. http://gerrit.cloudera.org:8080/#/c/21643/2/fe/src/main/java/org/apache/impala/planner/AnalyticPlanner.java@332 PS2, Line 332: Only add fully materialised slot descriptors Can you elaborate on this, please? Do we only add fully materialised slot descriptors because only they are needed by upper operators in the plan? And we can drop other slot descriptors? What it means if some child slots are materialized while others are not? http://gerrit.cloudera.org:8080/#/c/21643/2/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test File testdata/workloads/functional-query/queries/QueryTest/sort-complex.test: http://gerrit.cloudera.org:8080/#/c/21643/2/testdata/workloads/functional-query/queries/QueryTest/sort-complex.test@154 PS2, Line 154: # Regression test for # IMPALA-13272 Can we also add the following query: select row_no, str from ( select arr.small, arr.inner_struct1.str str, row_number() over (order by arr.inner_struct1.str) as row_no from functional_parquet.collection_struct_mix t, t.arr_contains_nested_struct arr ) res; And this as well: select row_no, str, small from ( select arr.small, arr.inner_struct1.str str, row_number() over (order by arr.inner_struct1.str) as row_no from functional_parquet.collection_struct_mix t, t.arr_contains_nested_struct arr ) res; -- To view, visit http://gerrit.cloudera.org:8080/21643 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I71d1fa28ad4ff2e1a8fc5b91d3fc271c33765190 Gerrit-Change-Number: 21643 Gerrit-PatchSet: 2 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 05 Aug 2024 13:27:41 +0000 Gerrit-HasComments: Yes
