[C++][Parquet] Performance regression in TypedColumnWriter::WriteBatch

[adamreeve]

Describe the bug, including details regarding any error messages, version, and platform.

When running our benchmarks for ParquetSharp against the new Arrow 13.0.0 release, one of the benchmarks was twice as slow compared to Arrow 12.0.1. Digging into this further, I found that the change that caused this slow down was #35230, and it appears that the reason for the slowdown is this particular benchmark makes a lot of calls to WriteBatch with very small batches, and there is now a lot of extra time spent looking up properties_->page_index_enabled(descr_->path()) in pages_change_on_record_boundaries().

It looks to me like the column properties shouldn't change one a TypedColumnWriter is constructed, so it should be fine to compute this value once on construction to fix this performance regression.

Component(s)

Parquet

[mapleFU]

Oh I see, I will try to fix this

[adamreeve]

Hi @mapleFU, no we haven't enabled the page index and we aren't using DATA_PAGE_V2, but I don't think your comment is correct, page_index_enabled is called when data page V2 is not used, as there is an OR condition:

  bool pages_change_on_record_boundaries() const {
    return properties_->data_page_version() == ParquetDataPageVersion::V2 ||
           properties_->page_index_enabled(descr_->path());
  }

I have tested a fix that moves the calculation of this to the constructor and that fixes the performance regression, so I was planning on making a PR for this soon but was just working on getting the tests to run locally.

[adamreeve]

My comment looks a bit silly now after your edit 😃. I had already tested a fix for this so have opened a PR with my change at #37454, but I'm happy for you to fix this instead if you see a better way as I'm not super familiar with the codebase.

[mapleFU]

Oh you've already submit a patch, I'm just trying to do the same thing, thanks a lot!