Java API change for supporting structs

[razajafri]

This is a very rough draft PR to tie down the interface change to support Structs for Parquet writer. Once we have the interface down, it's just a matter of coding in the rest of the pieces.

Here is how I envision it to be used by the end-user.

      ParquetWriterOptions options = ParquetWriterOptions.builder()
          .withColumnOptions(ParquetColumnWriterOptions.builder()
              .withColumnName("_c0")
              .withTimestampInt96(false)
              .withDecimalPrecision(4)
              .isNullable(true).build())
          .withStatisticsFrequency(ParquetWriterOptions.StatisticsFrequency.NONE)
          .withCompressionType(CompressionType.AUTO)
          .withMetadata("test", "test")
          .build();

I still don't know a good way to get rid of the old withColumnNames API on ParquetWriterOptions. We can't remove it as ORC is still using it. One option could be to just rip out the WriterOptions from the ParquetWriterOptions hierarchy.

[jlowe]

High-level idea seems OK to me as long as we flatten it when calling into JNI.

I still don't know a good way to get rid of the old withColumnNames API on ParquetWriterOptions.

We'd either need to disconnect it from the old one or have a compatibility mode where it translates the old values to new values if they were set via the old API, and of course the old API won't support structs. Setting options via both APIs would be an error. I'm OK if we want to ditch the old one, but it would be a breaking change.

[revans2]

A few things about the API.

1. I don't see a lot of value in requiring a builder for simple child columns.

ParquetWriterOptions options = ParquetWriterOptions.builder()
  .withSimpleColumn("column name 1")
  .withSimpleNullableColumn("column name 2")
  .withComplextColumn(ParquetColumnWriterOptions.builder()....

2. Should we add some kind of types to make the API more readable and less likely to let users point a gun at their foot?

The current C++ API lets anything go. i.e. Setting precision for non-decimal types and adding children to non-nested columns. Perhaps we could have something like.

ParquetWriterOptions options = ParquetWriterOptions.builder()
  .withColumn("column name 1")
  .withListColumn(ParquetColumnWriterOptions.builder()...)

One of the best ways I have found to do API design is to use the API.

Lets create a schema for a table that has a list of timestamps that should be output as int96.

     ParquetWriterOptions options = ParquetWriterOptions.builder()
         .withListColumn("_c0")
             .withTimestampData()
                 .withInt96()
                 .build()
             .build()
         .build();

Is that better than

      ParquetWriterOptions options = ParquetWriterOptions.builder()
          .withColumnOptions(
              ParquetColumnWriterOptions.builder()
                  .withColumnName("_c0").
                  .withColumnOptions(
                      ParquetColumnWriterOptions.builder()
                      .withTimestampInt96()
                      .build())
              .build())
          .build();

?

The second one is much more verbose, but I think it does a better job of matching up the build() with what it is building... Perhaps we can have a hybrid

    ParquetWriterOptions options = ParquetWriterOptions.builder()
        .withColumn(
             ParquetColumnWriterOptions.listBuilder(
                ParquetColumnWriterOptions.timestampBuilder()
                    .withInt96()
                    .build())
                .withColumnName("_c0")
                .build())
    .build();

[razajafri]

@revans2 @jlowe PTAL.

I think we need to get this in ASAP so it makes it to this release

[revans2]

I'm not sure we should be rushing this. I know you have deadlines but the current API is not clean and I'm not that excited about pushing one change at the end of a release knowing that we are likely going to have to make breaking changes to it in the next release.

[razajafri]

@devavret for cpp change

[razajafri]

@revans2 do you have any more questions?

[razajafri]

@revans2 @jlowe I have improved the API. PTAL

[razajafri]

@revans2 I have addressed your concerns. PTAL

[razajafri]

build

[razajafri]

rerun tests

[revans2]

I think it would be better to say that it adds a child column with the given name. and that should apply to all of the with methods.

[revans2]

Still not done

[revans2]

Unless we describe what the name is marking it as a param does nothing. This applies to all of the other java doc comments for params that are empty.

[revans2]

we need java docs here. They really need to describe how a list builder is different from a StructBuilder and that the name of the child is ignored.

[revans2]

I Still don't see it documented anywhere how a list builder works or that the name for it's children is ignored.

[razajafri]

CI is failing due to a known problem. There is a PR that was merged in rmm. This PR fixes this error, so we will have to wait until its merged to kick off the tests again

[revans2]

I am just done to nits in the comments.

[razajafri]

rerun tests

[razajafri]

@gpucibot merge