Fengling Wang has posted comments on this change. ( http://gerrit.cloudera.org:8080/9834 )
Change subject: KUDU-2371: Add WriteOptions class and ignoreNull option to upsertRows ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@7 PS2, Line 7: WriteOptio > Curious to hear others' opinions, but to me a name like 'treatNullAsUnset' Or maybe 'unsetNulls'? http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@7 PS2, Line 7: KUDU-2371 > I think you mean 2250? Done http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@9 PS2, Line 9: > nit: Try to wrap commit message lines at about 80 chars. Done http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@9 PS2, Line 9: s to allow > Ignoring nulls can apply equally well to any write type, not just upserts. i think it's good to also apply to updates but it doesn't make sense to me for inserts / deletes? http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@10 PS2, Line 10: s > nit: Add "the". Done http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@11 PS2, Line 11: more fields in the future. The instance of this class is passed to > This patch doesn't expose this functionality through DefaultSource.createRe I see. http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@12 PS2, Line 12: func > nit: "For example, this feature is useful..." Done http://gerrit.cloudera.org:8080/#/c/9834/2//COMMIT_MSG@13 PS2, Line 13: > nit: s/where/because Done http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala: http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@259 PS2, Line 259: > s/value/values Done http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/KuduContext.scala@326 PS2, Line 326: if (row.isNullAt(sparkIdx)) { > It's probably worth adding a check that the column isn't in the primary key Added. Not sure if it's the most effective way to check? http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala: http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala@43 PS2, Line 43: > If we keep adding options to modify the behavior of these operations we're Done http://gerrit.cloudera.org:8080/#/c/9834/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/OperationType.scala@43 PS2, Line 43: > On second thought it may be cleaner to just introduce a WriteOptions class Done -- To view, visit http://gerrit.cloudera.org:8080/9834 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ide908ea29f572849eca0ba850ee197c1b22a07c8 Gerrit-Change-Number: 9834 Gerrit-PatchSet: 3 Gerrit-Owner: Fengling Wang <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Fengling Wang <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Sun, 01 Apr 2018 17:26:16 +0000 Gerrit-HasComments: Yes
