First pass at support protobuf over the wire instead of Jackson-generated JSON from POJOs. Attempts to be non-destructive to those POJOs in an attempt to not break existing JSON support. Translation between POJO and protobuf pushed down to the POJOs (similar to how requests are presently dispatched to the service to create a response). This keeps translation between the two objects close to the "model": the POJOs.

Nowhere close to being ready to commit (no tests at all), putting it up for interested parties to take a glance and provide any early feedback to the approach so far.

Looks good at this point. I was surprised how large the patch is - but not your fault at all, I guess it's quite a big protocol. Nor is its protobuf's fault - everything seems to translate quite naturally.

In a few places you have default constructors that assign null to final members of type Map or List and I presume that protobuf uses Unsafe to re-assign them. Is it valid for users of those classes to assume that the Map and List members are not null?

And it would be good to fix CALCITE-839 before committing this. It seems wrong that any file would have both jackson and protobuf imports, and 839 would concentrate the jackson imports in a smaller number of files.

Please be sure to use 3rd person declarative for javadoc method comments.

Obviously files will need license headers and other fixup before commit.

It should be possible for some of the tests to be re-used between Protobuf and Jackson. Tests of the end-to-end service call variety. Such tests probably already exist. (There are thread-safety issues, see CALCITE-687, but I believe the tests are basically sound.)

Of course there will be other tests that are absolutely protobuf-specific.

Big thank you for taking a look, Julian. Much appreciated!

I was surprised how large the patch is - but not your fault at all, I guess it's quite a big protocol. Nor is its protobuf's fault - everything seems to translate quite naturally.

Heh, yeah, things exploded a bit trying to unwrap Frame . Which is also one bit that translated very poorly due to its List<Object>. If you look carefully here (and I think there was another place), I still use the Jackson serialization to just put JSON into a protocol buffer because I had no reasonable means to say what these {{Object}}s actually were. I think this could be improved but is perhaps not necessary for a first pass (I assume anyone leverage the protobuf endpoint has a reasonable JSON parser which can unwrap what the server sent it)

In a few places you have default constructors that assign null to final members of type Map or List and I presume that protobuf uses Unsafe to re-assign them. Is it valid for users of those classes to assume that the Map and List members are not null?

Yeah, I need to revisit these. I was trying to leverage a visitor pattern as much as possible to prevent the massive if/elseif blocks. This was one unintended consequence of it. I'm not 100% happy with it, but I'm not sure of a better fix at the moment. Will hopefully go away before a final patch.

And it would be good to fix CALCITE-839 before committing this. It seems wrong that any file would have both jackson and protobuf imports, and 839 would concentrate the jackson imports in a smaller number of files.

Agreed.

Please be sure to use 3rd person declarative for javadoc method comments.

Ok.

It should be possible for some of the tests to be re-used between Protobuf and Jackson. Tests of the end-to-end service call variety. Such tests probably already exist. (There are thread-safety issues, see CALCITE-687, but I believe the tests are basically sound.)

This was my hope as well. I've been working through tests for protobuf from low->high with the hopes that when I get to a high-level query, I can just replace the client and the Handler in the server and I can reuse the same test.

Could you do a union-like message as container for the different types that end up in the untyped list? It's a shame that relatively expensive JSON parsing is still needed presently.

Could you do a union-like message as container for the different types that end up in the untyped list? It's a shame that relatively expensive JSON parsing is still needed presently.

Yes, I think that would work (assuming I understand you correctly: make an "object" message that is one optional instance of every possible type). It would be ugly to use (since I don't think we'd have a concise way to populate that message without a big conditional block, and users would be forced to unwrap it too), but it's a better solution than what I'm doing at the moment I think

Agreed it's not pretty, but a step further in that all serialization would be binary.

Yeah, that's a good point. Thanks for mentioning it! I'll see if I can make that work.

Elsewhere I use a TypedValue struct. It's basically a switched union. Maybe you could use that.

v2

(update text taken from commit message)

Protobuf messages exist to support translation of the "API" POJOs that currently define Calcite's wire data. Consistent methods that encapsulate transformation from POJO to protobuf and back again. Also involved proper hashCode() and equals() implementations on the POJOs to support testing. POJOs maintained as a user-facing "model", with protobufs happening behind the curtain.

Unit tests added which verify object transformation, a "mock" service, a local service, and a remote service. Service-level tests were implementation with full parity against existing Avatica tests.

Need to cleanup duplicated code, fix checkstyle faults, run some "standalone" tests (using Phoenix, likely), and address some previously mentioned suggestions from Julian and Andrew.

Here's the patch "everyone" has been waiting for – yes, it's still massive.

Changes over the previous:

• No more embedded JSON in protobuf
• More tests
• Tried to cleanup Javadocs per Julian's earlier comment

Included some manual testing of PQS using the protobuf impl via sqlline with the same level of success as JSON (e.g. ignoring things like PHOENIX-1972).

I just noticed I need to do something about license headers on protobuf specs and generated code (I think Accumulo has a script I can lift which wraps the generation and adds headers and fun stuff).

Because this patch has been lagging along for some time now, I'm leaning against doing things like pulling off the Jackson annotations prior to saying "this is ready". I don't see the harm in having both Jackson and Protobuf imports in one place for some time. We know it's something to address in the future, but I don't see it as something that must be done before this gets applied IMO.

Newer patch which fixes missing licenses on generated code. Provides a little script that does a couple of nice things

• Adds license headers
• Adds suppress warnings annotation
• Check for correct protoc version

Can later be automated to run during generate-sources as a part of the build (if desired). Hadoop does have a maven-plugin that we could use, but this script does what we need (IMO) and keeps out another dependency.

Removing the jackson annotations should be a separate commit, but I think we should do it as part of this task. Otherwise when will it get done? Also, the client now depends on both protobuf and jackson - a client using the protobuf transport should not depend on jackson. I don't mind whether the commit is sequenced before or after the protobuf transport.

Another thing that needs to get done as part of this task is CALCITE-687. With intermittent test failures we have no confidence in the Avatica stack at present.

Regarding code generation. Which is simpler: committing generated code, or generating as part of the build? If either option does not require protobuf to be installed on the build machine (using e.g. apt-get install) that would be preferable.

Thanks for the feedback, Julian.

Removing the jackson annotations should be a separate commit, but I think we should do it as part of this task. Otherwise when will it get done?

I have a list of other things I'd like to address (PHOENIX-1972, CALCITE-871, CALCITE-645 are at the fore-front). My intent would be to do it while I continue work on those issues.

a client using the protobuf transport should not depend on jackson

That's a fair point. I've been limiting my view to users who are just consuming the JDBC driver instead of doing things by hand in Java. In the former case, everything is already shaded+relocated into the Avatica jar, and they don't have to care what they're getting. I could see us moving the present avatica module into some avatica-core module, and pulling protobuf and jackson specific code into their own modules. This would help reduce the dependency footprint.

With intermittent test failures we have no confidence in the Avatica stack at present

Yeah, this was a pain today trying to get the tests to pass in one build. I'd be happy to look into this one too, but I don't have a good feeling for where to start. Do you have any hunch where the concurrency issue is?

Which is simpler: committing generated code, or generating as part of the build

The former doesn't require us doing anything on build-boxes. Maven pulls in the right dependencies and we don't do any code-gen on a normal build – that's what this patch does (and is the large part of why it's so large). I think it's reasonable to assume that we're going to have decent stability in the protobuf classes and recompiling will be an uncommon event. The only downside is that you have to remember to recompile when you do change the proto files (which would become rather apparent quickly if you forget )

I'd be happy to look into this one too, but I don't have a good feeling for where to start. Do you have any hunch where the concurrency issue is?

Actually, ignore me. I don't need to create confusion by crossing the two issues. Will move relevant discussion to CALCITE-687

Jacques Nadeau, Since you use protoc in Drill, can you please review the protoc build aspects of this patch? Is it preferable to check in generated files, or to generate at build time (even if this means everyone who builds has to install protobuf)? Is generation best accomplished via scripts or via a maven profile?

Instead of another patch, created a PR with the previous changes rebase'd on top of the pending CALCITE-687 fixes.

That did expose one corner-case in the serialization of TypedValue which I resolved as well.

Hey Jacques Nadeau, any thought on Julian's earlier query? Having done both approaches previously, I think not automating the regeneration in the build process is the path of least resistance presently. It would be easy to automate this later if so desired (e.g. if/when protobuf takes over as the default serialization method).

Julian Hyde, are you OK with me pushing off the Jackson-annotation "separation" work (CALCITE-839) until this gets merged in? Additionally, I wanted to see if there was anything else I can do to help move this along. I'm pleased with how this has turned out for a first pass of work. There were some user-facing changes that I made which would be good to get an ACK on as we would want to keep them relatively stable.

Josh Elser, I'm OK deferring CALCITE-839 as long as there is a good faith intention to do it soon (preferably this release).

I would like to hear what Jacques Nadeau has to say about committing generated protobuf code, but again, I can live with either alternative as long as there is a commitment to change it if/when it starts to hurt.

If we go the path of committing generated code, we will at least need a "how to change protobuf" section in howto.md.

I will start a final review of your patch and will commit if everything's fine.

as long as there is a good faith intention to do it soon (preferably this release).

Great, yes the intention is there.

If we go the path of committing generated code, we will at least need a "how to change protobuf" section in howto.md.

Happy to provide.

I will start a final review of your patch and will commit if everything's fine.

Thanks, your efforts are much appreciated.

1. I fixed errors about unused dependencies, see https://github.com/julianhyde/incubator-calcite/tree/840-profobuf

2. In https://github.com/joshelser/incubator-calcite/commit/0f7086cdeb67a397666ed62a56f0da33ebb74640 did you intend to remote the Apache headers?

As generated files, what should the license of these files be?

3. You added several equals() methods, but you don't seem to check for 'this == o' before diving into a field-by-field check. This is the norm (e.g. java.lang.String and java.util.AbstractList do it), and in fact JsonSerive.finagle(Meta.Signature) relies on it for performance. Can you change these methods to this general pattern:

  if (o == this) {
    return true;
  }
  if (!(o instanceof TheType)) {
    return false;
  }
  ...

I fixed errors about unused dependencies

Hrm, curious. I wonder why mdep didn't fire errors for me.

Seeing all of the @Override changes that you made, is there something I should be setting in an editor/checkstyle plugin to achieve this or is it just ad-hoc style I should adopt?

In general, thanks for making all those changes in 9948b1cd57. I'll merge those into my branch and leave the squashing up to you (whether you preserve the attribution of your own efforts).

did you intend to remote the Apache headers

Oh, this was not intentional. I will push a fix for that.

As generated files, what should the license of these files be?

AFAIK, they should be treated no differently than hand-written Calcite code, but that's off the top of my head.

Can you change these methods to this general pattern

Ah, yeah, I forgot to do that short-circuit. I can update the POJOs to do that.

It's an ad hoc style. I don't think that checkstyle is able to check it; I use the extra.sh tool in https://github.com/julianhyde/share/tree/master/tools but I don't expect others to.

Merged in the changes you made and fixed the above issues you mentioned. The history is a little gross at the moment, but I can rebase the commits to clean things up to make it easier on you if you'd like, Julian.

I've already rebased in my branch. And I'm making fixes as I go along (e.g. javadoc errors when you run 'mvn site'.) I'll squash when it all works. Meantime, individual small commits are best - I can cherry-pick them.

Great, sounds like a plan.

Hey Guys,

Sorry I didn't see the query earlier. My theory: don't check in anything that is derived. Cruel Reality: Remove all barriers for new users. A number of new developers didn't read the instructions and then got stuck rather than solving (or reading the error message). We had an especially big problem because we were using PB 2.5 when everyone else was on 2.4. Everybody is on 2.5 now so we could probably stop including the files. That being said, the ease for new developers still trumps. With 3.x coming out, I'd recommend doing the same as we do on Drill. Specifically: create a separate module that is pb only and use a special profile to generate compilations (and add headers). I'd just pick up this: [1]. Note, you can remove the extra stuff for protostuf if you like. That being said, I would suggest that if you haven't already gone way down a path, consider using the protostuf generated files. They give you beans (instead of builders) as well as a nice transformation between json and protobuf. We use this in Drill to be able to move back and forth between json and protobuf as requirements dictate.

[1] https://github.com/apache/drill/blob/master/protocol/pom.xml

Almost there. I've squashed everything into https://github.com/julianhyde/incubator-calcite/commits/840-protobuf (not profobuf). Just need your modification to the HOWTO.

Just need your modification to the HOWTO.

Oops! I forgot about that. Pushed 1150a6b

Jacques Nadeau, thanks for the great write up. I appreciate your thoughts.

create a separate module that is pb only and use a special profile to generate compilations (and add headers)

I think Julian and I touched on this one already. In splitting up the jackson annotations, we both agreed that it would be good to move JSON stuff into one maven module and Protobuf stuff into another module. Thinking some more, we'll have to mess with the packaging a bit (instead of the shaded jar being generated in the avatica module). That's something we can work around, though.

That being said, I would suggest that if you haven't already gone way down a path, consider using the protostuf generated files

Hah, dang. A little too late. Not to say we can investigate further, but I've quite dealt with protobuf's builder pattern all over.

We use this in Drill to be able to move back and forth between json and protobuf as requirements dictate

My hope is that protobuf3 will include a good JSON serialization natively (and will help reduce the dependency footprint), but time will be the ultimate tell here.

Speaking of dependencies though, I need to update the LICENSE and NOTICE files too. Including protocol buffers means that the top-level L&N files are wrong as well as the L&N for the shaded calcite-avatica jar.

Pushed c3e54bd for the LICENSE update. NOTICE should be fine, afaik.

I'm trying to follow your instructions to re-generate protobuf files. How did you install protobuf? 3.0 has only just gone beta, so it seems that I need to build it from source.

How did you install protobuf? 3.0 has only just gone beta, so it seems that I need to build it from source

Ugh of course. Wasn't thinking. Let me expand on it.

bf21bd8 covers protobuf 3 installation

All squashed into julianhyde/master. If that passes the tests I'll commit to apache master.

Fixed in http://git-wip-us.apache.org/repos/asf/incubator-calcite/commit/cb7c213c. Thanks for the patch, Josh Elser!

Thanks for all of your help in getting this committed, Julian Hyde!

Resolved in release 1.5.0 (2015-11-10)