Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14690 )
Change subject: IMPALA-9146: Add a configurable limit for the size of broadcast input. ...................................................................... Patch Set 4: (3 comments) Code looks good, just had a few asks as far as additional tests. http://gerrit.cloudera.org:8080/#/c/14690/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/14690/4//COMMIT_MSG@30 PS4, Line 30: - Added a new unit test in PlannerTest that sets the Can you also add the option to query-options-test just to test the validations. I think you can just add it to the table of byte options in SetByteOptions() since its behaviour is consistent with the other options. http://gerrit.cloudera.org:8080/#/c/14690/4/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/14690/4/be/src/service/query-options.cc@914 PS4, Line 914: ParseMemValue(value, "broadcast bytes limit for join operations", &broadcast_bytes_limit)); > line too long (103 > 90) Assume you'll fix in next patch. You can run these checks locally the HEAD commit with: ./bin/jenkins/critique-gerrit-review.py --dryrun If you want to check whether they're resolved. Also ok to rely on the bot. http://gerrit.cloudera.org:8080/#/c/14690/4/testdata/workloads/functional-planner/queries/PlannerTest/broadcast-bytes-limit.test File testdata/workloads/functional-planner/queries/PlannerTest/broadcast-bytes-limit.test: PS4: Can you also add a negative test where it is below the threshold and doesn't switch to broadcast. And a negative test where it's above the threshold but the /* +broadcast */ hint overrides it. -- To view, visit http://gerrit.cloudera.org:8080/14690 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe5639ca38acb72e0194aa80bc6ebb6cafb2acd9 Gerrit-Change-Number: 14690 Gerrit-PatchSet: 4 Gerrit-Owner: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 19 Nov 2019 19:39:08 +0000 Gerrit-HasComments: Yes
