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 <ara...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 01 May 2019 22:43:16 +0000 Gerrit-HasComments: Yes
