Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.5.0
    • Component/s: c
    • Labels:
      None

      Description

      I've been working on a pretty major patch that changes the way the C library implements schema resolution. Before, we would compare the writer and reader schemas each time we try to read a record from an Avro file. This is a fair bit of wasted effort. The approach I'm taking with the new implementation is to separate schema resolution and binary parsing into separate operations. There's a new "consumer" API, which defines a set of callbacks for processing Avro data that conforms to a schema. The new avro_consume_binary function reads binary-encoded Avro data from a buffer or file, and passes that data into a consumer instance. Each consumer instance is associated with the writer schema of the data that it expects to process.

      Schema resolution is now implemented in the new avro_resolver_new function, which returns a consumer instance that knows how to translate from the writer schema to the reader schema. As the resolver receives data via the consumer API, it fills in the contents of a destination avro_datum_t (which should be an instance of the reader schema).

      This work isn't complete yet — I still have to implement promotion (int->long and friends), and have to add support for recursive schemas (via the AVRO_LINK schema type). But I wanted to get the patch out there for people to view and test in the meantime. This patch depends on a few other of my patches, that haven't made it into SVN yet; if you want to test the code without patching yourself, I have a tracking branch on github.

      1. 0003-Recursive-schema-resolution.patch
        30 kB
        Douglas Creager
      2. 0002-Promotion-of-values-during-schema-resolution.patch
        16 kB
        Douglas Creager
      3. 0001-Better-schema-resolution.patch
        70 kB
        Douglas Creager

        Activity

        Douglas Creager created issue -
        Douglas Creager made changes -
        Field Original Value New Value
        Attachment 0001-Better-schema-resolution.patch [ 12470725 ]
        Hide
        Douglas Creager added a comment -

        And here's a patch that implements the promotion rules.

        Show
        Douglas Creager added a comment - And here's a patch that implements the promotion rules.
        Douglas Creager made changes -
        Hide
        Douglas Creager added a comment -

        And lastly, here is a patch that works with recursive schemas. I believe that all of the schema resolution rules are implemented correctly now.

        Show
        Douglas Creager added a comment - And lastly, here is a patch that works with recursive schemas. I believe that all of the schema resolution rules are implemented correctly now.
        Douglas Creager made changes -
        Attachment 0003-Recursive-schema-resolution.patch [ 12470865 ]
        Douglas Creager made changes -
        Fix Version/s 1.5.0 [ 12315282 ]
        Priority Major [ 3 ] Blocker [ 1 ]
        Douglas Creager made changes -
        Status Open [ 1 ] Patch Available [ 10002 ]
        Hide
        Doug Cutting added a comment -

        I'm getting the following 2 test failures on Ubuntu 10.10:

        TEST /home/cutting/src/avro/trunk/lang/c/tests/schema_tests/fail/record_with_non
        array_fields.../bin/bash: line 5: 25436 Segmentation fault      ${dir}$tst
        FAIL: test_avro_schema
        
        + ../libtool execute valgrind --leak-check=full -q test_avro_data
        + grep -E ^==[0-9]+== 
        ==25529== 1,538 (32 direct, 1,506 indirect) bytes in 1 blocks are definitely los
        t in loss record 18 of 18
        ==25529==    at 0x4025BD3: malloc (vg_replace_malloc.c:236)
        ==25529==    by 0x4025C5D: realloc (vg_replace_malloc.c:525)
        ==25529==    by 0x8049888: test_allocator (test_avro_data.c:58)
        ==25529==    by 0x40319CB: avro_schema_record (schema.c:560)
        ==25529==    by 0x40329C0: avro_schema_from_json_t (schema.c:853)
        ==25529==    by 0x4032D46: avro_schema_from_json (schema.c:1104)
        ==25529==    by 0x804A3A4: test_nested_record (test_avro_data.c:402)
        ==25529==    by 0x804B09D: main (test_avro_data.c:661)
        ==25529== 
        + [ 0 -eq 0 ]
        + exit 1
        FAIL: test_valgrind
        
        Show
        Doug Cutting added a comment - I'm getting the following 2 test failures on Ubuntu 10.10: TEST /home/cutting/src/avro/trunk/lang/c/tests/schema_tests/fail/record_with_non array_fields.../bin/bash: line 5: 25436 Segmentation fault ${dir}$tst FAIL: test_avro_schema + ../libtool execute valgrind --leak-check=full -q test_avro_data + grep -E ^==[0-9]+== ==25529== 1,538 (32 direct, 1,506 indirect) bytes in 1 blocks are definitely los t in loss record 18 of 18 ==25529== at 0x4025BD3: malloc (vg_replace_malloc.c:236) ==25529== by 0x4025C5D: realloc (vg_replace_malloc.c:525) ==25529== by 0x8049888: test_allocator (test_avro_data.c:58) ==25529== by 0x40319CB: avro_schema_record (schema.c:560) ==25529== by 0x40329C0: avro_schema_from_json_t (schema.c:853) ==25529== by 0x4032D46: avro_schema_from_json (schema.c:1104) ==25529== by 0x804A3A4: test_nested_record (test_avro_data.c:402) ==25529== by 0x804B09D: main (test_avro_data.c:661) ==25529== + [ 0 -eq 0 ] + exit 1 FAIL: test_valgrind
        Hide
        Douglas Creager added a comment -

        Is this 32-bit or 64-bit? I've got a 64-bit Ubuntu 10.10 box, and don't get any segfault on that. If it's 32-bit, I'll spin up another VM and see if I can reproduce.

        The valgrind error is an example of AVRO-766. It's not a regression; there wasn't any test for it before. The memory leak only affects recursive schemas. I think fixing it will require a fairly major rewrite of the reference counting mechanism; is that something we want to block 1.5.0 for?

        Show
        Douglas Creager added a comment - Is this 32-bit or 64-bit? I've got a 64-bit Ubuntu 10.10 box, and don't get any segfault on that. If it's 32-bit, I'll spin up another VM and see if I can reproduce. The valgrind error is an example of AVRO-766 . It's not a regression; there wasn't any test for it before. The memory leak only affects recursive schemas. I think fixing it will require a fairly major rewrite of the reference counting mechanism; is that something we want to block 1.5.0 for?
        Hide
        Doug Cutting added a comment -

        32-bit.

        Show
        Doug Cutting added a comment - 32-bit.
        Hide
        Doug Cutting added a comment -

        I don't think we have to fix AVRO-766 for 1.5.0, but maybe we should turn off that test until it's fixed?

        Show
        Doug Cutting added a comment - I don't think we have to fix AVRO-766 for 1.5.0, but maybe we should turn off that test until it's fixed?
        Hide
        Douglas Creager added a comment -

        Just committed a fix for the segfault issue. I think it actually had to with the patch for AVRO-463, though I think I referenced this issue in the commit message. Hopefully this will resolve the buildbot failures on FreeBSD and Solaris, too.

        Show
        Douglas Creager added a comment - Just committed a fix for the segfault issue. I think it actually had to with the patch for AVRO-463 , though I think I referenced this issue in the commit message. Hopefully this will resolve the buildbot failures on FreeBSD and Solaris, too.
        Hide
        Doug Cutting added a comment -

        Segfault is fixed for me. Thanks!

        Should we turn off the failing valgrind test and resolve this issue as "fixed"?

        Show
        Doug Cutting added a comment - Segfault is fixed for me. Thanks! Should we turn off the failing valgrind test and resolve this issue as "fixed"?
        Hide
        Douglas Creager added a comment -

        The test will skip the valgrind portion if valgrind isn't installed...so yeah, we can comment out the valgrind test for now. I'll do that.

        Show
        Douglas Creager added a comment - The test will skip the valgrind portion if valgrind isn't installed...so yeah, we can comment out the valgrind test for now. I'll do that.
        Douglas Creager made changes -
        Status Patch Available [ 10002 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Doug Cutting made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

          • Assignee:
            Douglas Creager
            Reporter:
            Douglas Creager
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development