KITE-1081 FS writers to use the current view schema

[megnataraj]

I am not sure if you got a chance to look into my latest commit addressing all your comments. After adding this new commit, build is failing for CDH5 alone and looking into the logs it doesn't seem those errors were introduced by this new commit, I could be wrong.

[chris-hirstein]

@rdblue Have you had a chance to review the update that @megnataraj provided?

[rdblue]

@megnataraj and @chris-hirstein, I've looked at the update and it looks mostly good. My comments on TestAvroWriter#newWriter still applies though. That writer should use the schema that was passed in.

[chris-hirstein]

@rdblue It looks like @megnataraj added an update, can you take another look? Thanks!

[mkwhitacre]

Re-kicked off Travis build b/c failures seemed unrelated to the PR.

+1 for this PR pending successful build for non-flaky reasons.

[rdblue]

I'm not sure that the tests are exercising the correct code path, so I'd appreciate it if you didn't merge this until I have time to take a closer look. Sorry to be a blocker here.

[chris-hirstein]

@rdblue Will you have time to review this change soon?

[rdblue]

@chris-hirstein, @megnataraj, thanks for the reminder. I've posted a comment about making sure the tests cover what we need to. I hope that is clear enough but if it isn't please ask questions about it and I'll make sure they get answered.

[anandsagark]

I've updated this pull request as per the changes suggested by @rdblue Please take a look and see if everything looks ok to merge, the failure in the integration checks is not related to this PR

[rdblue]

@anandsagark, I'll look today.

[rbrush]

+1 overall. I'm willing to merge unless there are objections from @rdblue or @mkwhitacre.

(I might quibble that the TestValue class could do with a more descriptive name, like "ValueWithoutOptionalField", but I don't think that's enough to block it.)

[rdblue]

I don't think that the new test actually tests what it should. It looks like the schemas for the two values are the same. I think the dataset should have a 2-field record and the code should write a 1-field record.

But, I've been holding this up for too long and I'm fairly confident that it works, so I'm not going to shoot it down. Thanks for being patient, everyone.

[rbrush]

@rdblue Ugh, you're right. Got my wires crossed on which schema the view pointed to.

@anandsagark, if I'm reading this right I think this just means making testReaderWriterCompatibleSchema use a dataset that uses the "testDescriptor" rather than the "valueDescriptor" so we write to a different schema than the writer has.

[anandsagark]

@rdblue @rbrush I've updated PR to use different schemas for read, write. Let me know if this is in tune with what you guys are looking for.

edit : updated PR again, dataset created with 2-field record and writer used 1-field record.

[rdblue]

+1

Test looks good to me. Thanks @anandsagark

[rbrush]

I kicked off the build again (looks like there was a spurious failure on the default profile). I'll merge pending a clean build on default and CDH5 (I know the CDH4 profile is failing for unrelated reasons.)

[rbrush]

The build is clean modulo the known CDH4 issue and we have +1's. Merging.

Thanks everyone!