Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15176 )
Change subject: KUDU-3049: [spark] Automatic handling of schema drift ...................................................................... Patch Set 5: Code-Review+1 (2 comments) Looks this good to me, I just noticed that a few cases are not covered by the tests in Scala sqlContext. Not sure whether it's critical, but anyways wanted to point at that. http://gerrit.cloudera.org:8080/#/c/15176/5/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: http://gerrit.cloudera.org:8080/#/c/15176/5/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@572 PS5, Line 572: assertEquals(3, afterDf.schema.fields.length) paranoid nit: I didn't notice it in the first review round, but there isn't a precondition on the number of the columns in the table before attempting to insert the row with the new column. Of course, it's highly unlikely that the test table's structure ever changes to add 'val2' column, but to make things consistent in such unlikely case, maybe consider adding assertEquals(2, df.schema.fields.length) before line 569? http://gerrit.cloudera.org:8080/#/c/15176/5/java/kudu-spark/src/test/scala/org/apache/kudu/spark/kudu/DefaultSourceTest.scala@576 PS5, Line 576: Do you think it's worth adding a couple more scenarios: * try to have the scenario similar to insert a row with not-yet-existing column where handleSchemaDrift is not set (i.e. testing the backward compatibility of the schema drift change)? * a scenario when one actor adds a new column with one type, while another actor (i.e. different sqlContext) tries to insert a row with new column of the same name, but of different type right after that? -- To view, visit http://gerrit.cloudera.org:8080/15176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib1edebb293d6ae79c26a0ecb9ce7755308f667f4 Gerrit-Change-Number: 15176 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 11 Feb 2020 21:04:30 +0000 Gerrit-HasComments: Yes
