arrow icon indicating copy to clipboard operation
arrow copied to clipboard

GH-40868: [C++][Parquet] Minor: Remove "Experimental" for parquet::RecordReader

Open mapleFU opened this issue 1 year ago • 7 comments

Rationale for this change

Remove experimental for parquet::internal::RecordReader

What changes are included in this PR?

Remove experimental for parquet::internal::RecordReader

Are these changes tested?

no

Are there any user-facing changes?

no

  • GitHub Issue: #40868

mapleFU avatar Mar 29 '24 14:03 mapleFU

:warning: GitHub issue #40868 has been automatically assigned in GitHub to PR creator.

github-actions[bot] avatar Mar 29 '24 14:03 github-actions[bot]

I just remove the experimental but not internal::, the later one will introduce lots compilation error. But will do it if you like

mapleFU avatar Mar 29 '24 14:03 mapleFU

I would prefer fixing those compile errors at the same time. We have to do that any way.

wgtmac avatar Mar 29 '24 14:03 wgtmac

/arrow/cpp/src/parquet/file_reader.h:65:33: error: declaration of 'std::shared_ptr<parquet::RecordReader> parquet::RowGroupReader::RecordReader(int, bool)' changes meaning of 'RecordReader' [-fpermissive]
   65 |   std::shared_ptr<RecordReader> RecordReader(int i, bool read_dictionary = false);
      |                                 ^~~~~~~~~~~~
/arrow/cpp/src/parquet/file_reader.h:40:7: note: 'RecordReader' declared here as 'class parquet::RecordReader'
   40 | class RecordReader;
      |       ^~~~~~~~~~~~
/arrow/cpp/src/parquet/file_reader.h:90:31: error: type/value mismatch at argument 1 in template parameter list for 'template<class _Tp> class std::shared_ptr'
   90 |   std::shared_ptr<RecordReader> RecordReaderWithExposeEncoding(
      |                               ^

Hmmm

mapleFU avatar Apr 01 '24 05:04 mapleFU

@wgtmac I tried to removed the internal, but RowGroupReader::RecordReader is a function, which conflicts here. This patch I'll first remove experimental here.

mapleFU avatar Jul 03 '24 02:07 mapleFU

IMHO, it is literally weird to remove experimental before removing internal namespace. The expectation is that the API is stable after removing experimental except that we will include the namespace change in the same release.

wgtmac avatar Jul 03 '24 02:07 wgtmac

Then we may need to change this interface:

  // EXPERIMENTAL: Construct a RecordReader for the indicated column of the row group.
  // Ownership is shared with the RowGroupReader.
  std::shared_ptr<internal::RecordReader> RecordReader(int i,
                                                       bool read_dictionary = false);

I'm also ok to change this to GetRecordReader

mapleFU avatar Jul 03 '24 03:07 mapleFU