Skip to content

Comments

GH-3401: Remove redundant MAP_KEY_VALUE annotation at middle level of logical type map#3402

Open
HuaHuaY wants to merge 1 commit intoapache:masterfrom
HuaHuaY:remove_map_annotation
Open

GH-3401: Remove redundant MAP_KEY_VALUE annotation at middle level of logical type map#3402
HuaHuaY wants to merge 1 commit intoapache:masterfrom
HuaHuaY:remove_map_annotation

Conversation

@HuaHuaY
Copy link

@HuaHuaY HuaHuaY commented Feb 24, 2026

Rationale for this change

Remove redundant MAP_KEY_VALUE annotation which is not required in parquet's spec.

What changes are included in this PR?

ConversionPatterns will not pass LogicalTypeAnnotation.MapKeyValueTypeAnnotation to GroupType function.

Are these changes tested?

Yes.

Are there any user-facing changes?

Yes. The MAP_KEY_VALUE annotation at middle level of thrift structure map<..., ...> will be removed.

Closes #3401

Copilot AI review requested due to automatic review settings February 24, 2026 08:09
@HuaHuaY HuaHuaY changed the title GH-3401: Remove redundant MAP_KEY_VALUE annotation GH-3401: Remove redundant MAP_KEY_VALUE annotation at middle level of logical type map Feb 24, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the redundant MAP_KEY_VALUE annotation from newly generated Parquet schemas to comply with the Parquet format specification. According to the spec, MAP_KEY_VALUE is a legacy annotation for backward compatibility with ConvertedType and should not be used in new schemas. The change affects schema generation while preserving backward compatibility for reading existing schemas.

Changes:

  • Removed MAP_KEY_VALUE annotation from ConversionPatterns.mapType() method
  • Updated documentation in Types.java and package-info.java to reflect the removal
  • Updated test expectations across Thrift and Avro modules to match new schema format

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
parquet-column/src/main/java/org/apache/parquet/schema/ConversionPatterns.java Core implementation: Removed MAP_KEY_VALUE annotation from mapType() method when creating the repeated key_value group
parquet-column/src/main/java/org/apache/parquet/schema/Types.java Updated documentation comments to remove MAP_KEY_VALUE annotation from example schemas
parquet-avro/src/main/java/org/apache/parquet/avro/package-info.java Updated package documentation to remove MAP_KEY_VALUE annotation from map type descriptions in conversion tables
parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java Updated test expectations to verify schemas without MAP_KEY_VALUE annotation
parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverterProjectUnion.java Updated test expectations for union projection tests to reflect removal of MAP_KEY_VALUE
parquet-avro/src/test/java/org/apache/parquet/avro/TestAvroSchemaConverter.java Updated Avro schema conversion test expectations to match new format

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@HuaHuaY HuaHuaY force-pushed the remove_map_annotation branch from 66e6fe1 to 5971e79 Compare February 24, 2026 08:23
@wgtmac
Copy link
Member

wgtmac commented Feb 24, 2026

I think this change makes sense because the Parquet spec does not require the middle layer to use MAP_KEY_VALUE annotation. Do you have any concern with backward compatibility? @gszadovszky @Fokko?

@gszadovszky
Copy link
Contributor

I think this change makes sense because the Parquet spec does not require the middle layer to use MAP_KEY_VALUE annotation. Do you have any concern with backward compatibility? @gszadovszky @Fokko?

I don't have any concerns, @wgtmac. LGTM.

@wgtmac
Copy link
Member

wgtmac commented Feb 24, 2026

Thanks @gszadovszky! I'll leave it open for a few days just in case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Added redundant MAP_KEY_VALUE annotation to parquet's thrift structure

3 participants