Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14503 )
Change subject: WIP: fix ORDERED scan regression when scanned columns are out-of-order ...................................................................... Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/14503/1/src/kudu/common/schema.cc File src/kudu/common/schema.cc: http://gerrit.cloudera.org:8080/#/c/14503/1/src/kudu/common/schema.cc@706 PS1, Line 706: void SchemaBuilder::SortByReferenceSchema(const Schema& reference) { Is it the entire order of the schema that we care about? Or just the keys? If it's just the keys, could we just pull the keys from the reference schema, remove them from `cols_` by name, and then add them to the front? Correct me if i'm wrong, I'm assuming the broken invariant shows up in Schema::Compare(), where it's comparing the first num_key_columns_ columns http://gerrit.cloudera.org:8080/#/c/14503/1/src/kudu/common/schema.cc@721 PS1, Line 721: // Such columns are ignored for the purposes of the sort, though their : // relative order is preserved thanks to stable_sort. Are we guaranteed that non-existent columns are not a part of the key range? http://gerrit.cloudera.org:8080/#/c/14503/1/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: http://gerrit.cloudera.org:8080/#/c/14503/1/src/kudu/tserver/tablet_service.cc@2338 PS1, Line 2338: // There's no guarantee that the order of columns in the projection matches : // the order of columns in the tablet schema. This is a problem for ORDERED : // scans, which use the projection's key columns to decode rowset bounds, and : // those bounds are always in tablet schema column order. In such cases we : // must sort the projection using the tablet schema, otherwise the decoding : // will fail with either a "key too short" or "missing separator" error. nit: can you move this down by the ORDERED condition, since this only really seems relevant there? Also it might be clearer to give an obvious example of why the order might be off, e.g. "There's no guarantee that the projection column order matches the tablet schema's column order (e.g. if the primary keys were not a part of the project but we've manually added the primary keys because this is an ORDERED scan). This is a problem for ORDERED scans..." -- To view, visit http://gerrit.cloudera.org:8080/14503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb213f466c7d01b6953a863d8aa53a34b5ac8893 Gerrit-Change-Number: 14503 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Fri, 18 Oct 2019 01:26:06 +0000 Gerrit-HasComments: Yes
