Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/18676 )
Change subject: IMPALA-11430: Support custom hash schema for Kudu range tables ...................................................................... Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/18676/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18676/11//COMMIT_MSG@11 PS11, Line 11: to > nit: drop duplicate 'to' Done http://gerrit.cloudera.org:8080/#/c/18676/11//COMMIT_MSG@23 PS11, Line 23: , > I haven't yet looked into the rest of the code, but I'm curious: is the syn I made additional changes to make commas optional in the rest of the partitioning syntax. http://gerrit.cloudera.org:8080/#/c/18676/11/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test File testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test: http://gerrit.cloudera.org:8080/#/c/18676/11/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test@324 PS11, Line 324: hash partitions 10 > Is this a valid syntax at all? What hash schema was assumed for the new ra The syntax is valid and fails as expected since the number of levels differs. http://gerrit.cloudera.org:8080/#/c/18676/11/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test@347 PS11, Line 347: '30 <= VALUES < 40 HASH(id) PARTITIONS 2 HASH(c2) PARTITIONS 3' > Does it makes sense to add a range with hash schema with reverse order of h Changed existing case to have this ordering difference. http://gerrit.cloudera.org:8080/#/c/18676/11/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test@377 PS11, Line 377: '30 <= VALUES < 40 HASH(id,c2) PARTITIONS 2' > Isn't it supposed to be the table-wide hash schema in this case, i.e. 'HASH The hash parameters at both the table and partition level default to the PK definition if not specified. http://gerrit.cloudera.org:8080/#/c/18676/11/testdata/workloads/functional-query/queries/QueryTest/kudu_partition_ddl.test@380 PS11, Line 380: ==== > Does it make sense to also add the following cases to check how the error i added testcases. The PARTITIONS 0 and PARTITIONS 1 cases were flakey, sometimes returning an error and sometimes being accepted and timing out in Kudu. Those cases will need to be fixed in Kudu before negative tests can be added. -- To view, visit http://gerrit.cloudera.org:8080/18676 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I981056e0827f4957580706d6e73742e4e6743c1c Gerrit-Change-Number: 18676 Gerrit-PatchSet: 11 Gerrit-Owner: Kurt Deschler <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Fri, 15 Jul 2022 02:41:57 +0000 Gerrit-HasComments: Yes
