GH-41764: [Parquet][C++] Support future logical types in the Parquet reader

[paleolimbot]

Rationale for this change

If new logical types are added to Parquet, how should they be handled? (Or is it easy enough to just support them as soon as they are added to the spec?)

What changes are included in this PR?

Experimental ones!

Are these changes tested?

Absolutely not!

Are there any user-facing changes?

Not yet!

• GitHub Issue: [Parquet][C++] Behaviour of unknown logical type when encountered in Parquet reader #41764

[github-actions]

⚠️ GitHub issue #41764 has been automatically assigned in GitHub to PR creator.

[wgtmac]

I agree that we might need an option to make it throw or ignore for unknown logical type.

BTW, it seems that parquet-java also fails on unknown type: https://github.com/apache/parquet-java/blob/6809a18597e56709696af58313343aeea93f0b99/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L1142-L1179

[mapleFU]

Should we regard it as physical type with unknown logical type

[wgtmac]

UNKNOWN logical type means all values are null, IIUC.

[paleolimbot]

Hmm...it looks like there was already a concept for this (that was used for unknown legacy converted types): the UndefinedLogicalType(). I'm not sure how to test this, though!

[mapleFU]

I also prefer UndefinedLogicalType. I checked arrow-rs that it also raise error in this condition

[paleolimbot]

Any ideas on how to craft a test file with an unknown type?

[wgtmac]

arrow/cpp/src/parquet/metadata.cc

Line 1920 in 13630c7

ToParquet(static_cast<parquet::schema::GroupNode*>(schema_->schema_root().get()),

You might need to hack the thrift object directly from metadata_->schema above by setting an unknown logical value. This means we have to create a parquet file any way.

[emkornfield]

If we don't throw here at the very least we need to ignore statistics for the logical type (e.g. I believe sort order for float16 doesn't correspond to signed comparison)

[paleolimbot]

If we don't throw here at the very least we need to ignore statistics for the logical type

I did some investigation and I believe statistics are already ignored for UndefinedLogicalType:

arrow/cpp/src/parquet/types.cc

Lines 324 to 332 in 93712bf

	SortOrder::type GetSortOrder(const std::shared_ptr<const LogicalType>& logical_type,
	Type::type primitive) {
	SortOrder::type o = SortOrder::UNKNOWN;
	if (logical_type && logical_type->is_valid()) {
	o = (logical_type->is_none() ? DefaultSortOrder(primitive)
	: logical_type->sort_order());
	}
	return o;
	}

arrow/cpp/src/parquet/metadata.cc

Lines 254 to 272 in 93712bf

	inline bool is_stats_set() const {
	DCHECK(writer_version_ != nullptr);
	// If the column statistics don't exist or column sort order is unknown
	// we cannot use the column stats
	if (!column_metadata_->__isset.statistics ||
	descr_->sort_order() == SortOrder::UNKNOWN) {
	return false;
	}
	if (possible_stats_ == nullptr) {
	possible_stats_ = MakeColumnStats(*column_metadata_, descr_);
	}
	EncodedStatistics encodedStatistics = possible_stats_->Encode();
	return writer_version_->HasCorrectStatistics(type(), encodedStatistics,
	descr_->sort_order());
	}
	inline std::shared_ptr<Statistics> statistics() const {
	return is_stats_set() ? possible_stats_ : nullptr;
	}

(i.e., ColumnChunkMetadata::statistics() will return nullptr if logical_type->is_valid() returns false)

I still have to figure out how to write a full-on file so that this is tested properly! (I figured out how to test the schema conversion but not how to check a full-on file yet).

[emkornfield]

I still have to figure out how to write a full-on file so that this is tested properly! (I figured out how to test the schema conversion but not how to check a full-on file yet).

Probably hacking the generated thrift code with a very high field ID and writing out the file and then trying to read it back in could work?

[mapleFU]

Just ping me if this is ready

[paleolimbot]

Just ping me if this is ready

Thanks! I think the missing piece is an actual end-to-end test with a real Parquet file (and I haven't had time to circle back to the suggestion of hacking the generated thrift for an actual written-to-disk Parquet file).

[mapleFU]

default False?