Avro
  1. Avro
  2. AVRO-648

Use a template library for the SpecificCompiler

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5.0
    • Component/s: java
    • Labels:
      None
    • Hadoop Flags:
      Reviewed

      Description

      This JIRA proposes uses a templating library instead of string concatenation for the SpecificCompiler.

      We've had conversations on the list about customizing the generated code (by adding getters and setters, adding utility classes for the arrays, adding String-friendly (as opposed to Utf8) accessors, etc.), but we've been stymied by the fact that the specific compiler is hard-coded to use one template, and it's hard to experiment with. Sam Pullara (at http://github.com/spullara/avrocompiler) has done pretty much this: he forked/subclassed a copy of SpecificCompiler that uses the Mustache language to generate code. He's also gone ahead and done some of the customizations.

      In the patch I'm about to post, I've replicated the existing code generation using Velocity. We already build Velocity for some of the IPC plugins, and it's an Apache project. The existing tests pass, plus I've added tests that check that the generated code is character-for-character the same, in a handful of cases. This was actually quite painful, since I had to reproduce some questionable indentation and trailing whitespace . That said, I'm pleased with how easy it was to incorporate the templates.

      Eventually, I hope we support getters and setters, or perhaps support multiple versions of templates. (If someone wants to generate, say, C++ code, the path is now a lot easier for that, as well.)

      1. AVRO-648.patch
        40 kB
        Doug Cutting
      2. AVRO-648.patch.txt
        37 kB
        Philip Zeyliger

        Activity

        Hide
        Philip Zeyliger added a comment -

        Here's a preliminary patch. I'd still like to diff the generated directory from the tests against a version without this patch, and I know that I haven't added license headers to the templates, and probably to the new tests.

        Show
        Philip Zeyliger added a comment - Here's a preliminary patch. I'd still like to diff the generated directory from the tests against a version without this patch, and I know that I haven't added license headers to the templates, and probably to the new tests.
        Hide
        Doug Cutting added a comment -

        Bravo! This is a great contribution!

        A few comments:

        • there are number of whitespace/formatting changes , making the patch harder to read, since one must scan each of them to see if anything's actually changed.
        • perhaps we could name the set of templates, e.g., org/apache/avro/specific/templates/{$name}/*.vm, and make the name a parameter of the compiler. then the existing templates might be named 'classic' or 'simple' and we might then later add, e.g., 'bean' templates that generate accessor methods? Or do you intend that as a subsequent contribution?
        Show
        Doug Cutting added a comment - Bravo! This is a great contribution! A few comments: there are number of whitespace/formatting changes , making the patch harder to read, since one must scan each of them to see if anything's actually changed. perhaps we could name the set of templates, e.g., org/apache/avro/specific/templates/{$name}/*.vm, and make the name a parameter of the compiler. then the existing templates might be named 'classic' or 'simple' and we might then later add, e.g., 'bean' templates that generate accessor methods? Or do you intend that as a subsequent contribution?
        Hide
        Philip Zeyliger added a comment -

        I'll take a look at regenerating the patch with less whitespace later on. As you can tell, I was working on it rather late, and excited that I got the tests to pass

        Pluggability for a template directory is the natural next patch. (Eventually, we'll need templating or pluggability for filenames, too, since the deep nested directory hierarchies are a Java thing: I have in mind as a use case a "doc" generator a la javadoc, though maybe javadoc is good enough.)

        BTW, I've now posted the patch at https://review.cloudera.org/r/762/diff/#index_header , if you want to see it side-by-side.

        – Philip

        Show
        Philip Zeyliger added a comment - I'll take a look at regenerating the patch with less whitespace later on. As you can tell, I was working on it rather late, and excited that I got the tests to pass Pluggability for a template directory is the natural next patch. (Eventually, we'll need templating or pluggability for filenames, too, since the deep nested directory hierarchies are a Java thing: I have in mind as a use case a "doc" generator a la javadoc, though maybe javadoc is good enough.) BTW, I've now posted the patch at https://review.cloudera.org/r/762/diff/#index_header , if you want to see it side-by-side. – Philip
        Hide
        Doug Cutting added a comment -

        Marking this for 1.5.0 so we don't forget about it.

        Show
        Doug Cutting added a comment - Marking this for 1.5.0 so we don't forget about it.
        Hide
        Doug Cutting added a comment -

        Here's a new version of the patch that:

        • removes whitespace-only changes
        • places templates in a java/classic directory, preparing for future variations of java and other langs
        • permits setting the template directory by a method or a system property
        • makes SpecificCompiler easily subclassible, to provide additional methods to templates
        • adds the apache license to template files
        Show
        Doug Cutting added a comment - Here's a new version of the patch that: removes whitespace-only changes places templates in a java/classic directory, preparing for future variations of java and other langs permits setting the template directory by a method or a system property makes SpecificCompiler easily subclassible, to provide additional methods to templates adds the apache license to template files
        Hide
        Scott Carey added a comment -

        I'd like to get this committed soon, since work on AVRO-647 is likely to conflict with this ticket more than most, and I have some time to work on that again.

        Show
        Scott Carey added a comment - I'd like to get this committed soon, since work on AVRO-647 is likely to conflict with this ticket more than most, and I have some time to work on that again.
        Hide
        Philip Zeyliger added a comment -

        +1. Sorry 'bout the slow review; crazy couple of days.

        Show
        Philip Zeyliger added a comment - +1. Sorry 'bout the slow review; crazy couple of days.
        Hide
        Doug Cutting added a comment -

        I just committed this. Thanks, Philip!

        Show
        Doug Cutting added a comment - I just committed this. Thanks, Philip!

          People

          • Assignee:
            Philip Zeyliger
            Reporter:
            Philip Zeyliger
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development