You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Abhishek Rawat (Code Review)" <ge...@cloudera.org> on 2019/05/01 00:10:35 UTC
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Abhishek Rawat has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13193
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
IMPALA-7293: Show tuple layout in explain plan.
This change prints materialized tuple's layout in the explain plan, at
explain_level >= 2.
New logic was added in the Planner::getExplainString(..) to print the
tuple layouts. The logic walks through the TupleDescriptors (and
SlotDescriptors) related to a given PlannerContext (for a given query),
stored in the associated global state (Analyzer.globalState_.descTbl
object) and prints materialized tuples and slots.
Testing:
- Tons of existing test units in PlannerTest were updated to include
the tuple layout. The existing test cases provide good coverage.
- New testunit testTupleLayout was added to PlannerTest.java. The new
test unit adds scenarios involving scalar types, complex types, joins,
order-by, aggregation, union all and subqueries.
Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
---
M fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test
M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
A testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
21 files changed, 4,878 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/13193/3
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
fe tests ran clean: https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/5447/console
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 May 2019 00:11:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
I like the new layout.
http://gerrit.cloudera.org:8080/#/c/13193/4/fe/src/main/java/org/apache/impala/catalog/StructType.java
File fe/src/main/java/org/apache/impala/catalog/StructType.java:
http://gerrit.cloudera.org:8080/#/c/13193/4/fe/src/main/java/org/apache/impala/catalog/StructType.java@108
PS4, Line 108: public String getExplainString(int depth) {
Could maybe do with a one-sentence comment explaining why we don't print contents.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 May 2019 00:33:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 4:
I'll promote to +2 once you decide what to do about Csaba's comments and update the tests.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 10 May 2019 18:35:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 4:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/3120/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 May 2019 03:29:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
(3 comments)
The change looks good to me. The comments are about ideas to make the output more readable (for us, as I think that no one else than Impala developers and tests will read these lines). Our eyes will appreciate a good layout if we will read it a lot in the future. :)
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
File testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test:
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@53
PS3, Line 53: Slot 10 offset=44 type=INT path=tpcds.store_sales.ss_quantity nullable=true
: Slot 11 offset=48 type=DECIMAL(7,2) path=tpcds.store_sales.ss_wholesale_cost nullable=true
These tables could be nicer to look at if the columns would be aligned with some padding. My suggestion is to move path to the last column, as it has the most variable length, and pad other columns to some same length, e.g. 2 digits for Slot id, 3 for offset and 14 for type. The main benefit would be that columns names from the same db + table would be always aligned.
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@55
PS3, Line 55: true
Maybe replace true/false with 1/0 just to make it shorter?
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@56
PS3, Line 56: type=
"type=" seems redundant to me - the SQL types with capital letters are recognizable enough.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 May 2019 23:18:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
(10 comments)
I have addressed the review comments. Patch #4 makes the necessary code changes for proper explain layout. I have however only updated the "tuple-layout.test" for now. Changing all the tests is rather extensive and I want to first get input on the layout in patch #4. Once we agree on a proper layout I can make the changes in all test units.
http://gerrit.cloudera.org:8080/#/c/13193/3/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/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@76
PS3, Line 76: public static final OffsetComparator OffsetComparator_ = new OffsetComparator();
> you can probably do:
Done
http://gerrit.cloudera.org:8080/#/c/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@331
PS3, Line 331: expBuilder.append("Slot " + getId() + " ");
> Consider using Objects.toStringHelper, eg:
Decided to use a new format where we have a tuple header
{Tuple=<id>, byteSize=<bytes>, path=<tuple_path>, Slot format=<offset, nullable, type, pth>}
The slot field names which we were earlier printing for every slot are now only printed once per tuple in the tuple header.
http://gerrit.cloudera.org:8080/#/c/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@333
PS3, Line 333: getType
> Would be better to call toSql() or whatever other method explicitly instead
I have added a getExplainString() for the Type class.
http://gerrit.cloudera.org:8080/#/c/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@334
PS3, Line 334: expBuilder.append("path=" + getPath() + " ");
> Would be better to call getExplainString() or whatever other method explici
Added Path::getExplainString().
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@78
PS3, Line 78: Tuple 0:
> Might be worth including the tuple byte size in the header? It's otherwise
Done
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@79
PS3, Line 79: Slot 0 offset=0 type=ARRAY<STRUCT<o_orderkey:BIGINT,o_orderstatus:STRING,o_totalprice:DECIMAL(12,2),o_orderdate:STRING,o_orderpriority:STRING,o_clerk:STRING,o_shippriority:INT,o_comment:STRING,o_lineitems:ARRAY<STRUCT<l_partkey:BIGINT,l_suppkey:BIGINT,l_linenumber:INT,l_quantity:DECIMAL(12,2),l_extendedprice:DECIMAL(12,2),l_discount:DECIMAL(12,2),l_tax:DECIMAL(12,2),l_returnflag:STRING,l_linestatus:STRING,l_shipdate:STRING,l_commitdate:STRING,l_receiptdate:STRING,l_shipinstruct:STRING,l_shipmode:STRING,l_comment:STRING>>>> path=c.c_orders nullable=true
> As discussed offline, I think we want to avoid including the type of the ne
Nested/Complex Structs are now being represented as STRUCT<...>
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@80
PS3, Line 80: Slot 2 offset=12 type=BIGINT path=c.c_custkey nullable=true
> Maybe we can drop the "Slot <x>" prefix to make it a little denser? I don't
I ended up dropping Slot <x> and other filed names. The tuple header is also changed as follows:
Tuple=<id> byteSize=<sz> path=<tuple_path> Slot Format=<offset nullable type relative-path>
The "path.relative-path" returns the proper path for a slot. This was done because for most slots the path had a common prefix (TupleDescriptor's path)
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
File testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test:
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@53
PS3, Line 53: Slot 10 offset=44 type=INT path=tpcds.store_sales.ss_quantity nullable=true
: Slot 11 offset=48 type=DECIMAL(7,2) path=tpcds.store_sales.ss_wholesale_cost nullable=true
> These tables could be nicer to look at if the columns would be aligned with
Good point. I added padding for better readability and moved the repeated filed names to Tuple's header. Also moved path to the end. Please look at the new changes in "tuple-layout.test"
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@55
PS3, Line 55: true
> Maybe replace true/false with 1/0 just to make it shorter?
Done
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test@56
PS3, Line 56: type=
> "type=" seems redundant to me - the SQL types with capital letters are reco
Done. Also the other field names were also redundant and so moved them to the Tuple's header.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 May 2019 00:27:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Abhishek Rawat (Code Review)" <ge...@cloudera.org>.
Abhishek Rawat has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
IMPALA-7293: Show tuple layout in explain plan.
This change prints materialized tuple's layout in the explain plan, at
explain_level >= 2.
New logic was added in the Planner::getExplainString(..) to print the
tuple layouts. The logic walks through the TupleDescriptors (and
SlotDescriptors) related to a given PlannerContext (for a given query),
stored in the associated global state (Analyzer.globalState_.descTbl
object) and prints materialized tuples and slots.
Testing:
- Tons of existing test units in PlannerTest were updated to include
the tuple layout. The existing test cases provide good coverage.
- New testunit testTupleLayout was added to PlannerTest.java. The new
test unit adds scenarios involving scalar types, complex types, joins,
order-by, aggregation, union all and subqueries.
Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
---
M fe/src/main/java/org/apache/impala/analysis/DescriptorTable.java
M fe/src/main/java/org/apache/impala/analysis/Path.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/TupleDescriptor.java
M fe/src/main/java/org/apache/impala/catalog/ArrayType.java
M fe/src/main/java/org/apache/impala/catalog/MapType.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-planner/queries/PlannerTest/fk-pk-join-detection.test
M testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/max-row-size.test
M testdata/workloads/functional-planner/queries/PlannerTest/min-max-runtime-filters.test
M testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering-disabled.test
M testdata/workloads/functional-planner/queries/PlannerTest/parquet-filtering.test
M testdata/workloads/functional-planner/queries/PlannerTest/partition-pruning.test
M testdata/workloads/functional-planner/queries/PlannerTest/resource-requirements.test
M testdata/workloads/functional-planner/queries/PlannerTest/sort-expr-materialization.test
M testdata/workloads/functional-planner/queries/PlannerTest/spillable-buffer-sizing.test
M testdata/workloads/functional-planner/queries/PlannerTest/tablesample.test
A testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test
M testdata/workloads/functional-planner/queries/PlannerTest/union.test
26 files changed, 4,932 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/13193/4
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
(5 comments)
I'm at the point where I'd personally find this useful to understand the system and debug issues. I have a few suggestions for changes, some which we discussed offline before.
I'm not sure if others are likely to have a strong opinion - I feel like we can always iterate on it if it's missing things or people find things they don't like abou tit.
http://gerrit.cloudera.org:8080/#/c/13193/3/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/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@333
PS3, Line 333: getType
Would be better to call toSql() or whatever other method explicitly instead of relying on implicit toString(). Generally toString() returns debugging output, not output appropriate for end-users.
http://gerrit.cloudera.org:8080/#/c/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@334
PS3, Line 334: expBuilder.append("path=" + getPath() + " ");
Would be better to call getExplainString() or whatever other method explicitly instead of relying on implicit toString().
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@78
PS3, Line 78: Tuple 0:
Might be worth including the tuple byte size in the header? It's otherwise a little tricky to infer how big it is with padding and null bit on the end? Ok to ignore if it'll make it too noisy.
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@79
PS3, Line 79: Slot 0 offset=0 type=ARRAY<STRUCT<o_orderkey:BIGINT,o_orderstatus:STRING,o_totalprice:DECIMAL(12,2),o_orderdate:STRING,o_orderpriority:STRING,o_clerk:STRING,o_shippriority:INT,o_comment:STRING,o_lineitems:ARRAY<STRUCT<l_partkey:BIGINT,l_suppkey:BIGINT,l_linenumber:INT,l_quantity:DECIMAL(12,2),l_extendedprice:DECIMAL(12,2),l_discount:DECIMAL(12,2),l_tax:DECIMAL(12,2),l_returnflag:STRING,l_linestatus:STRING,l_shipdate:STRING,l_commitdate:STRING,l_receiptdate:STRING,l_shipinstruct:STRING,l_shipmode:STRING,l_comment:STRING>>>> path=c.c_orders nullable=true
As discussed offline, I think we want to avoid including the type of the nested struct. It's noisy and doesn't match the physical representation (since fields may not be materialised).
It still makes sense to include types of nested arrays and maps.
http://gerrit.cloudera.org:8080/#/c/13193/3/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test@80
PS3, Line 80: Slot 2 offset=12 type=BIGINT path=c.c_custkey nullable=true
Maybe we can drop the "Slot <x>" prefix to make it a little denser? I don't think the slot IDs are particularly useful.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 May 2019 22:43:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 4: Code-Review+1
(4 comments)
I have a few more suggestions for the layout, feel free to ignore them if you disagree or do not want to spend more time with this.
http://gerrit.cloudera.org:8080/#/c/13193/4/testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test
File testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test:
http://gerrit.cloudera.org:8080/#/c/13193/4/testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test@28
PS4, Line 28: missing stats
The missing stats lead to different explain output, which will cause failed tests during the Jenkins build.
http://gerrit.cloudera.org:8080/#/c/13193/4/testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test@219
PS4, Line 219: Slot format=<offset nullable type relative-path>
Maybe write this only once before the first tuple?
http://gerrit.cloudera.org:8080/#/c/13193/4/testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test@220
PS4, Line 220: 0 1
I would prefer a smaller padding to make the lines more compact. I wouldn't care about very large offsets, e.g. above 9999 - as the offsets are ordered, so most row will naturally aligned even without padding.
http://gerrit.cloudera.org:8080/#/c/13193/4/testdata/workloads/functional-planner/queries/PlannerTest/tuple-layout.test@221
PS4, Line 221: ARRAY<ARRAY<INT>> array_array_col
An idea to complicate the layout further: we could make the scalar case more compact + print the whole complex types by creating a new column like "complex-type" after the path. It would be empty for scalars, and the 3rd column for collections could be simply COLLECTION.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 4
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 08 May 2019 09:33:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13193/3/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/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@76
PS3, Line 76: public static final OffsetComparator OffsetComparator_ = new OffsetComparator();
you can probably do:
public static final Comparator<SlotDescriptor> COMPARE_BY_OFFSET =
Ordering.natural().onResultOf(SlotDescriptor::getByteOffset);
(using guava's Ordering utility)
http://gerrit.cloudera.org:8080/#/c/13193/3/fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java@331
PS3, Line 331: expBuilder.append("Slot " + getId() + " ");
Consider using Objects.toStringHelper, eg:
return Objects.toStringHelper("Slot")
.add("offset", getByteOffset())
.add(type, getType()
.toString();
which results in a string like 'Slot{offset=1, type=INT}', etc. and it has handy methods to skip null, etc.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 03 May 2019 05:49:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7293: Show tuple layout in explain plan.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/13193 )
Change subject: IMPALA-7293: Show tuple layout in explain plan.
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/3020/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/13193
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iac38290217a219e75400e4206835992a7ea31dba
Gerrit-Change-Number: 13193
Gerrit-PatchSet: 3
Gerrit-Owner: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 01 May 2019 01:09:51 +0000
Gerrit-HasComments: No