[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. cfile: clean up encoding-test to use fewer templates This test made heavy use of templates, which made things overly complicated and hard to follow. All of the block builders/decoders already implement common interfaces, so we can use runtime polymorphism instead for the majority of code here. Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Reviewed-on: http://gerrit.cloudera.org:8080/15044 Reviewed-by: Andrew Wong Tested-by: Andrew Wong --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h 5 files changed, 270 insertions(+), 320 deletions(-) Approvals: Andrew Wong: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 7 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 6: Verified+1 Failed test was a tracked, unrelated flaky test: KUDU-2616 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 22:11:16 + Gerrit-HasComments: No
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has removed a vote on this change. Change subject: cfile: clean up encoding-test to use fewer templates .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has removed a vote on this change. Change subject: cfile: clean up encoding-test to use fewer templates .. Removed -Verified by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 20:46:01 + Gerrit-HasComments: No
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 5 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 09 Apr 2020 01:21:52 + Gerrit-HasComments: No
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 4: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: PS3: > I took a peek at it, changing the signature of functions in this interface Ack -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Apr 2020 23:15:38 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Bankim Bhavsar has uploaded a new patch set (#4) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. cfile: clean up encoding-test to use fewer templates This test made heavy use of templates, which made things overly complicated and hard to follow. All of the block builders/decoders already implement common interfaces, so we can use runtime polymorphism instead for the majority of code here. Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h 5 files changed, 270 insertions(+), 320 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/15044/4 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 4 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: PS3: > Oops, yea. I took a peek at it, changing the signature of functions in this interface would entail changing function pointers and what appears bunch of templatized specialization for encodings. It's best done separately as a follow-up change. http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h@53 PS3, Line 53: // iter parameter will only be used when it is dictionary encoding > nit: not your fault, but could you update this to mention that this is an i Done -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 08 Apr 2020 22:11:28 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: PS3: > you mean the other way around, right? Oops, yea. -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 07 Apr 2020 22:28:41 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: PS3: > nit: while you're here, would you mind flipping the arguments of the builde you mean the other way around, right? http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h@46 PS1, Line 46: > nit: while you are here, maybe stick * and & to the type in the changes lin Done -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 07 Apr 2020 22:24:31 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 3: Code-Review+1 (2 comments) http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: PS3: nit: while you're here, would you mind flipping the arguments of the builders to the out-params come before the input-params? https://google.github.io/styleguide/cppguide.html#Output_Parameters http://gerrit.cloudera.org:8080/#/c/15044/3/src/kudu/cfile/type_encodings.h@53 PS3, Line 53: // iter parameter will only be used when it is dictionary encoding nit: not your fault, but could you update this to mention that this is an input parameter that points to the parent CFile in order for blocks to share the same vocabulary? Or maybe just rename this to parent_cfile_iter or something -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Apr 2020 20:35:44 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Bankim Bhavsar has uploaded a new patch set (#3) to the change originally created by Todd Lipcon. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. cfile: clean up encoding-test to use fewer templates This test made heavy use of templates, which made things overly complicated and hard to follow. All of the block builders/decoders already implement common interfaces, so we can use runtime polymorphism instead for the majority of code here. Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h 5 files changed, 266 insertions(+), 316 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/15044/3 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h@46 PS1, Line 46: > nit: while you are here, maybe stick * and & to the type in the changes lin Done -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Bankim Bhavsar Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 07 Apr 2020 00:55:41 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15044 to look at the new patch set (#2). Change subject: cfile: clean up encoding-test to use fewer templates .. cfile: clean up encoding-test to use fewer templates This test made heavy use of templates, which made things overly complicated and hard to follow. All of the block builders/decoders already implement common interfaces, so we can use runtime polymorphism instead for the majority of code here. This also cleans up the block builder/decoder factories to use unique_ptr out-parameters instead of raw pointers. Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a --- M src/kudu/cfile/binary_dict_block.cc M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h 8 files changed, 274 insertions(+), 321 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/15044/2 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15044 ) Change subject: cfile: clean up encoding-test to use fewer templates .. Patch Set 1: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h File src/kudu/cfile/type_encodings.h: http://gerrit.cloudera.org:8080/#/c/15044/1/src/kudu/cfile/type_encodings.h@46 PS1, Line 46: * nit: while you are here, maybe stick * and & to the type in the changes lines? -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 16 Jan 2020 05:52:49 + Gerrit-HasComments: Yes
[kudu-CR] cfile: clean up encoding-test to use fewer templates
Hello Alexey Serbin, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/15044 to review the following change. Change subject: cfile: clean up encoding-test to use fewer templates .. cfile: clean up encoding-test to use fewer templates This test made heavy use of templates, which made things overly complicated and hard to follow. All of the block builders/decoders already implement common interfaces, so we can use runtime polymorphism instead for the majority of code here. This also cleans up the block builder/decoder factories to use unique_ptr out-parameters instead of raw pointers. Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a --- M src/kudu/cfile/cfile_reader.cc M src/kudu/cfile/cfile_reader.h M src/kudu/cfile/cfile_writer.cc M src/kudu/cfile/cfile_writer.h M src/kudu/cfile/encoding-test.cc M src/kudu/cfile/type_encodings.cc M src/kudu/cfile/type_encodings.h 7 files changed, 239 insertions(+), 289 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/15044/1 -- To view, visit http://gerrit.cloudera.org:8080/15044 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Iba4464c2ea41107df96c68ea61576a0ea269277a Gerrit-Change-Number: 15044 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Alexey Serbin
