Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/4491 )
Change subject: KUDU-1563. Add support for INSERT IGNORE ...................................................................... Patch Set 17: (5 comments) http://gerrit.cloudera.org:8080/#/c/4491/17//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/4491/17//COMMIT_MSG@9 PS17, Line 9: Adds `INSERT IGNORE' operation which behaves like a : normal `INSERT' except in the case when a duplicate : row error would be raised by the primary key having been : previously inserted. Hmm, I thought the feedback from Dan and MJ in KUDU-1563 was to broaden the kinds of errors ignored from just duplicate row keys to potentially all per-row errors. This includes missing rows for future UPDATE/DELETE IGNORE operations, missing range partitions, and missing values for non-nullable columns without default values. I'd be in favor of that, especially because it makes the behavior of INSERT IGNORE more intuitive: it ignores errors of all kinds. I'm less certain about how configurable this needs to be. I think a reasonable first cut would be an operation that ignores _every error_; per-session configuration that restores certain errors for all INSERT IGNORE ops in that session could come later. What do you think about defining the operation such that it ignores all errors, then spending a bit of time hunting down additional per-row errors server-side and squelching them if INSERT IGNORE was the desired operation? Is that going to be a mountain of work? http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h File src/kudu/client/write_op.h: http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h@a187 PS17, Line 187: Nit: unintended delete? We separate classes by two lines in this file. http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/client/write_op.h@163 PS17, Line 163: Nit: add an extra line here. http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/row_op.h File src/kudu/tablet/row_op.h: http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/row_op.h@107 PS17, Line 107: // True if an INSERT IGNORE was ignored Move this up to the other bools, so that less padding is generated for each RowOp. http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/tablet_metrics.h File src/kudu/tablet/tablet_metrics.h: http://gerrit.cloudera.org:8080/#/c/4491/17/src/kudu/tablet/tablet_metrics.h@48 PS17, Line 48: scoped_refptr<Counter> insert_errors_ignored; Nit: maybe insertions_errors_ignored and move it down near insertions_failed_dup_key? -- To view, visit http://gerrit.cloudera.org:8080/4491 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5bfc35e9d27bd5e2d3375b68e6e4716ed671f36c Gerrit-Change-Number: 4491 Gerrit-PatchSet: 17 Gerrit-Owner: Brock Noland <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Brock Noland <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 04 Dec 2018 00:08:31 +0000 Gerrit-HasComments: Yes
