Dan Burkert has posted comments on this change.

Change subject: KUDU-1533 Spark Kudu Rdd/Dataframe upsert
......................................................................


Patch Set 2:

(8 comments)

This is looking great.   Sorry I've been so slow on reviews.  I'd love to get 
this in for the 0.10 release which is branching on Friday.  I'll try to be much 
faster on reviews, will you have time to work on it before then?

http://gerrit.cloudera.org:8080/#/c/3871/2//COMMIT_MSG
Commit Message:

Line 13: for Kudu without support for table truncation.
My understanding is that it's more fundamental than that: it requires table 
truncation *and* table creation if the table does not exist.  Doing the table 
creation without creation args (replication, partitioning, etc.) is not 
something I think we can support.


Line 19: The change is backwards-incompatible. Syntax like
Could you add this or something like it to docs/release_notes.adoc?


http://gerrit.cloudera.org:8080/#/c/3871/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala
File 
java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/DefaultSource.scala:

PS2, Line 172: insert
I liked your idea of defaulting to upsert, but allowing insert via a special 
configuration flag.  Can you bring that back from the previous patch?  I think 
in the long run most people will want upsert by default.


http://gerrit.cloudera.org:8080/#/c/3871/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:

Line 132:     writeRows(data, tableName, table => table.newInsert())
this is a fantastic way to reduce the boilerplate.


PS2, Line 179: writeRowPartition
how about 'writePartitionRows'


Line 180:                 schema: StructType,
indent


http://gerrit.cloudera.org:8080/#/c/3871/2/java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala
File java/kudu-spark/src/main/scala/org/apache/kudu/spark/kudu/package.scala:

Line 35
I thought this would still work because of the RelationProvider impl?  Or is it 
tied to CreatableRelationProvider?


http://gerrit.cloudera.org:8080/#/c/3871/2/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala
File 
java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala:

Line 75:     val tableName = "testcreatetable"
thank you!


-- 
To view, visit http://gerrit.cloudera.org:8080/3871
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib8e0d50fb74dc2ce5e757e8a56fc1e863f699822
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Chris George <chris.geo...@rms.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to