Implement initial support for avro logical types (#6482)

[tpn]

Description

This includes the date type. Scaffolding has been put in place to handle the other logical types. This closes #6482.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[rapids-bot]

Pull requests from external contributors require approval from a rapidsai organization member with write or admin permissions before CI can begin.

[tpn]

Looks like I don't have permission to edit the labels for this PR, maybe because I created it off an issue that I didn't raise?

gh pr edit 12788 --add-label "feature request,non-breaking"
Message: tpn does not have the correct permissions to execute `AddLabelsToLabelable`, Locations: [{Line:1 Column:54}]

[vyasr]

/ok to test

[vyasr]

Thanks for the contribution @tpn! @vuule and @galipremsagar are probably the best people to answer the open questions here.

[tpn]

Ignore the 80f28de force-push, I'm about to squash and remove the time-millis/micros stuff.

[tpn]

@vyasr can I get another /ok to test? Thanks! (I can't do this, right?)

And friendly ping to @vuule & @galipremsagar for review. I'd like to get this PR merged so I can tackle a couple of follow-up avro items. Thanks!

[vuule]

/ok to test

[vuule]

@tpn I'm OOTO this week, will review as soon as I can next week.
Apologies for the delay.

[tpn]

@tpn I'm OOTO this week, will review as soon as I can next week.
Apologies for the delay.

No problem, thanks for the update.

[kkraus14]

Would maybe be good to have some nulls mixed into a variant of this test as well to ensure that we properly handle the situations where we get a UNION type with nulls and dates.

[tpn]

Just pushed a change that rewrites the test to test nulls where applicable. Can you take a look? Thanks!

[tpn]

I like that idea. On Feb 21, 2023, at 10:24, Keith Kraus ***@***.***> wrote:  @kkraus14 commented on this pull request.

________________________________ In python/cudf/cudf/tests/test_avro_reader_fastavro_integration.py<#12788 (comment)>:

+ dates = [

+ datetime.date(1970, 1, 1), + datetime.date(1970, 1, 2), + datetime.date(1981, 10, 25), + datetime.date(2012, 5, 18), + datetime.date(2019, 9, 3), + ] Would maybe be good to have some nulls mixed into a variant of this test as well to ensure that we properly handle the situations where we get a UNION type with nulls and dates. — Reply to this email directly, view it on GitHub<#12788 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAEZZYWOJIBE22ZGCAXOVE3WYUB4FANCNFSM6AAAAAAU5VMOJY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[tpn]

Friendly ping to @vuule for another /ok to test and PR review.

I just pushed a rebase + some test tweaks that @kkraus14 suggested.

[vuule]

@tpn looks like the Java failure is related to this PR, not a random issue

[davidwendt]

The Java tests are here: https://github.com/rapidsai/cudf/blob/branch-23.04/java/src/test/java/ai/rapids/cudf/TableTest.java#L973-L1030
And the avro file used there is here: https://github.com/rapidsai/cudf/blob/branch-23.04/java/src/test/resources/alltypes_plain.avro

[tpn]

The Java tests are here: https://github.com/rapidsai/cudf/blob/branch-23.04/java/src/test/java/ai/rapids/cudf/TableTest.java#L973-L1030 And the avro file used there is here: https://github.com/rapidsai/cudf/blob/branch-23.04/java/src/test/resources/alltypes_plain.avro

Ah! I'll investigate.

[tpn]

Yeah it looks like either the Java tests or the Java avro test file will need some tweaking. Still investigating, but finishing up for the week shortly. I'll pick it up first thing next week.

[tpn]

I think I need some help with the Java test failures. If I run df = cudf.read_avro('alltypes_plain.avro') on a fresh cudf env vs my branch, I get identical dataframes, dtypes, everything. The only thing that stands out about the schema of the test avro file is the logicalType: 'timestamp-micros', but that type isn't supported in either the existing main or my branch:

[ins] In [22]: pprint(schema)
{'fields': [{'name': 'id', 'type': ['int', 'null']},
            {'name': 'bool_col', 'type': ['boolean', 'null']},
            {'name': 'tinyint_col', 'type': ['int', 'null']},
            {'name': 'smallint_col', 'type': ['int', 'null']},
            {'name': 'int_col', 'type': ['int', 'null']},
            {'name': 'bigint_col', 'type': ['long', 'null']},
            {'name': 'float_col', 'type': ['float', 'null']},
            {'name': 'double_col', 'type': ['double', 'null']},
            {'name': 'date_string_col', 'type': ['bytes', 'null']},
            {'name': 'string_col', 'type': ['bytes', 'null']},
            {'name': 'timestamp_col',
             'type': [{'logicalType': 'timestamp-micros', 'type': 'long'},
                      'null']}],
 'name': 'topLevelRecord',
 'type': 'record'}

I'm still trying to get a local Java env set up -- the cudfjar helper in build.sh doesn't appear to work (appears to be using obsolete nvidia-docker invocations?), and an explicit maven build appears to be failing with some bizarre codehaus error:

org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal org.codehaus.gmaven:gmaven-plugin:1.5:execute (setproperty) on project cudf: Execution setproperty of goal org.codehaus.gmaven:gmaven-plugin:1.5:execute failed: A required class was missing while executing org.codehaus.gmaven:gmaven-plugin:1.5:execute: Could not initialize class org.codehaus.groovy.runtime.InvokerHelper

[davidwendt]

Try running mvn test -B -DCUDF_JNI_ARROW_STATIC=OFF -DCUDF_JNI_ENABLE_PROFILING=OFF from your cpp/java directory perhaps. I have a local conda environment where I do builds on my machine and this worked for me.

Reference:

cudf/ci/test_java.sh

Line 41 in 55ed347

mvn test -B -DCUDF_JNI_ARROW_STATIC=OFF -DCUDF_JNI_ENABLE_PROFILING=OFF

[tpn]

Try running mvn test -B -DCUDF_JNI_ARROW_STATIC=OFF -DCUDF_JNI_ENABLE_PROFILING=OFF from your cpp/java directory perhaps. I have a local conda environment where I do builds on my machine and this worked for me.

Reference:

cudf/ci/test_java.sh

Line 41 in 55ed347

mvn test -B -DCUDF_JNI_ARROW_STATIC=OFF -DCUDF_JNI_ENABLE_PROFILING=OFF

Hey @davidwendt, yeah, that didn't seem to work either. Fails with the exact same codehaus error. I've just done a build.sh clean and am trying: export EXTRA_CMAKE_ARGS="-DCUDF_USE_ARROW_STATIC=ON -DCUDF_ENABLE_ARROW_S3=OFF"; bash build.sh -g -v libcudf to see if that yields something that maven will consume.

Don't suppose you're able to share your local conda env + build.sh invocation? Feel free to email if that's easier: trent at trent dot me.

[vuule]

/ok to test

[davidwendt]

I don't think I will be able to help you with the Java debugging. However, I was able to recreate the error with a C++ test. Here is the source:

#include <cudf_test/base_fixture.hpp>
#include <cudf_test/column_utilities.hpp>
#include <cudf_test/cudf_gtest.hpp>
#include <cudf/io/avro.hpp>
#include <cudf/io/datasource.hpp>
#include <string>
#include <vector>

struct AvroTest : public cudf::test::BaseFixture {};
TEST_F(AvroTest, TestFile)
{
  std::string filepath = "/cudf/java/src/test/resources/alltypes_plain.avro";
  std::vector<std::string> names({"bool_col", "int_col", "timestamp_col"});
  auto in_opts = cudf::io::avro_reader_options::builder(cudf::io::source_info{filepath})
                   .columns(names).build();
  auto result     = cudf::io::read_avro(in_opts);
  const auto view = result.tbl->view();
  for (auto col : view) {
    cudf::test::print(col);
  }
}
CUDF_TEST_PROGRAM_MAIN()

It is odd we do not have any avro gtests for this.
To build it, add a line to the cpp/tests/CMakeList.txt:

ConfigureTest(AVRO_TEST io/avro_test.cpp)

(Here the test file is cpp/tests/io/avro_test.cpp)
Once it is built you can run it from the build directory using gtests/AVRO_TEST
This should crash on the cudf::io::read_avro() line with a cudaErrorIllegalAddress error.
Running with compute-sanitizer will show it fails in the gpuDecodeAvroColumnData kernel:

========= COMPUTE-SANITIZER
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from AvroTest
[ RUN      ] AvroTest.TestFile
========= Invalid __global__ write of size 4 bytes
=========     at 0x1ca0 in cudf::io::avro::gpu::gpuDecodeAvroColumnData(cudf::device_span<const cudf::io::avro::block_desc_s, (unsigned long)18446744073709551615>, cudf::io::avro::gpu::schemadesc_s *, cudf::device_span<const thrust::pair<const char *, int>, (unsigned long)18446744073709551615>, const unsigned char *, unsigned int, unsigned int, unsigned long, unsigned long)
=========     by thread (0,0,0) in block (0,0,0)
=========     Address 0x0 is out of bounds
=========     and is 140,629,891,874,816 bytes before the nearest allocation at 0x7fe6f2c00000 of size 816 bytes
=========     Saved host backtrace up to driver entry point at kernel launch time
...

I'm not sure why it fails only with your new code. One thing I did notice that was different with the Python tests is that this one passes in column names. If you comment out passing in the column names in the avro_test.cpp then it runs without error

  auto in_opts = cudf::io::avro_reader_options::builder(cudf::io::source_info{filepath})
                   .build();  //.columns(names).build();

One last thing I noticed is that the compute-sanitizer error indicates it is trying to write 4 bytes to address 0x0 which would be a null-pointer. The only null-pointer I see here is the global_dictionary. So something is going wrong around that.

I'm not sure if that helps. I certainly can provide better help with the C++ test/debug. I'm confident that if this is fixed that the Java test will work as well.

[tpn]

I don't think I will be able to help you with the Java debugging. However, I was able to recreate the error with a C++ test. Here is the source:

Oh this is incredibly helpful, thanks! Investigating now.

[tpn]

Found the issue, heh, @davidwendt, it was an artifact of that switch statement refactoring I did. I also noticed a subtle logic error whilst traipsing through the code in cuda-gdb related to kind/logical_kind and type_union. Here's the patch. I'm going to pick it up tomorrow and rebase etc.

diff --git a/cpp/src/io/avro/avro_gpu.cu b/cpp/src/io/avro/avro_gpu.cu
index b6f56cbb5c..4d33d3f0e7 100644
--- a/cpp/src/io/avro/avro_gpu.cu
+++ b/cpp/src/io/avro/avro_gpu.cu
@@ -97,6 +97,8 @@ avro_decode_row(schemadesc_s const* schema,
       }
       if (i >= schema_len || skip_after < 0) break;
       kind = schema[i].kind;
+      logical_kind = schema[i].logical_kind;
+      if (is_supported_logical_type(logical_kind)) { kind = static_cast<type_kind_e>(logical_kind); }
       skip = skip_after;
     }
 
@@ -110,13 +112,17 @@ avro_decode_row(schemadesc_s const* schema,
         break;
 
       case type_int: {
-        int64_t v                           = avro_decode_zigzag_varint(cur, end);
-        static_cast<int32_t*>(dataptr)[row] = static_cast<int32_t>(v);
+        int64_t v = avro_decode_zigzag_varint(cur, end);
+        if (dataptr != nullptr && row < max_rows) {
+          static_cast<int32_t*>(dataptr)[row] = static_cast<int32_t>(v);
+        }
       } break;
 
       case type_long: {
-        int64_t v                           = avro_decode_zigzag_varint(cur, end);
-        static_cast<int64_t*>(dataptr)[row] = v;
+        int64_t v = avro_decode_zigzag_varint(cur, end);
+        if (dataptr != nullptr && row < max_rows) {
+          static_cast<int64_t*>(dataptr)[row] = v;
+        }
       } break;
 
       case type_bytes: [[fallthrough]];

Semi-related question: I'd like to add in the C++ unit tests for the alltypes_plain.avro file. It doesn't seem like there's a precedent for e.g. resolving the .avro file name relative to the unit test .cpp file name, so I was going to use something like this:

#include <string>
#include <experimental/filesystem>
namespace fs = std::experimental::filesystem;
...
TEST_F(AvroTest, TestFile)
{
  fs::path basedir = fs::path(__FILE__).parent_path();
  fs::path avro_path = basedir / "../../../java/src/test/resources/alltypes_plain.avro";
  auto srcinfo = cudf::io::source_info{avro_path.string()};
  ...

However, depending on std::experimental::filesystem seems like it might be undesirable, if not banned outright. Thoughts? (I suspect one of the reasons there aren't any C++ avro unit tests currently is that we don't have any C++ avro writer scaffolding, so we can't write .avro files to the test temp dir like we do with other file formats (which would avoid this issue of needing to reference a source-file-relative test file).)

[davidwendt]

I'm glad you found the issue. I'll let @vuule answer the question about testing against an existing test file in our repo.

[vuule]

Great news @tpn !
Could this be a Python test instead? We already have a precedent there both for Avro tests and for tests that use test files (not generated at run time).

As for std::filesystem, we already rely on it in a few places. Not sure about C++14, but it's not experimental as of C++17, which we currently use.

[tpn]

Great news @tpn ! Could this be a Python test instead? We already have a precedent there both for Avro tests and for tests that use test files (not generated at run time).

Actually yeah I need to figure out why the existing Python unit tests didn't hit this. There shouldn't be any reason preventing this logic from being exercised via Python avenue. I'll investigate.

As for std::filesystem, we already rely on it in a few places. Not sure about C++14, but it's not experimental as of C++17, which we currently use.

Ah maybe it's `std::filesystem::path`` I'm thinking of that's still experimental. Nevertheless, probably moot if I can get the Python tests to exercise this stuff.

[tpn]

Just rebased and pushed a fix for the CUDA kernel crashes. Figured out how to replicate the crash via a Python unit test (and verified it actually did crash prior to this fix being applied).

Friendly request for (hopefully the last!) /ok to test.

Edit: pushed again to fix a small typo.

[wence-]

/ok to test

[tpn]

Hurrah! Thanks folks for all your assistance in getting this merged. Much appreciated!