Skip to content

Core, OpenAPI: Preserve avg value sizes in REST JSON - #17855

Merged
szehon-ho merged 8 commits into
apache:mainfrom
huan233usc:content-file-parser-avg-value-sizes
Sep 4, 2026
Merged

Core, OpenAPI: Preserve avg value sizes in REST JSON#17855
szehon-ho merged 8 commits into
apache:mainfrom
huan233usc:content-file-parser-avg-value-sizes

Conversation

@huan233usc

@huan233uschuan233usc commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #17451. Average value sizes were dropped when content files crossed the planned REST scan JSON boundary.

Preserve avgValueSizes for data and delete files in ContentFileParser JSON round trips. REST JSON follows v4 field statistics: optional content-stats keyed by field ID, with avg-value-size-in-bytes on each per-field struct. Document the same shape in the REST OpenAPI schema.

This is file-level REST metadata only. It does not add manifest-level aggregation of average sizes.

Test plan

  • ./gradlew :iceberg-core:test --tests org.apache.iceberg.TestContentFileParser --tests org.apache.iceberg.TestFileScanTaskParser --tests org.apache.iceberg.TestDataTaskParser
  • ./gradlew :iceberg-core:spotlessCheck

AI Disclosure

Keep average value sizes in REST content-file JSON round trips and drop stale path-size statistics when rewrite_table_path rewrites position deletes.
Generated-by: Cursor
Comment threadopen-api/rest-catalog-open-api.yaml Outdated
Comment on lines +5126 to +5143
IntegerMap:
type: object
properties:
keys:
type: array
items:
$ref: '#/components/schemas/IntegerTypeValue'
description: "List of integer column ids for each corresponding value"
values:
type: array
items:
$ref: '#/components/schemas/IntegerTypeValue'
description: "List of integer values, matched to 'keys' by index"
example:
{
"keys": [ 1, 2 ],
"values": [ 8, 16 ]
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't we use ValueMap ? like what we use for other stats

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

updated the OpenAPI to match v4 field statistics (content-stats + avg-value-size-in-bytes)

Comment threadopen-api/rest-catalog-open-api.yaml Outdated
Comment on lines +5244 to +5249
avg-value-sizes:
allOf:
- $ref: '#/components/schemas/IntegerMap'
description:
Map of column id to average value size in memory (uncompressed) in
bytes over non-null values

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would this be part of data file ? like lower / upper bound are ?

@singhpk234
singhpk234 requested a review from nastraAugust 28, 2026 05:25
@huan233uschuan233usc changed the title Core, OpenAPI: Preserve avg value sizes across rewrites[WIP]Core, OpenAPI: Preserve avg value sizes across rewritesAug 28, 2026
Leave REST JSON round-trip changes here; rewrite_table_path handling moves to a separate PR.
Generated-by: Cursor
@huan233uschuan233usc changed the title [WIP]Core, OpenAPI: Preserve avg value sizes across rewritesCore, OpenAPI: Preserve avg value sizes in REST JSONAug 28, 2026
@huan233usc
huan233usc marked this pull request as draft August 28, 2026 05:45
@huan233usc

Copy link
Copy Markdown
ContributorAuthor

Hi @singhpk234 for quick review -- I just realize that this PR was not marked as draft when I am double checking the generated code vs spec. Will asked for another review when I finish my self validation. Sry for confusion

Xin Huang added 6 commits August 27, 2026 22:53
avg_value_size_in_bytes lives in v4 content_stats field structs, not a v1-v3 metrics map. REST JSON now uses content-stats keyed by field ID.
Generated-by: Cursor
Use the spec wording for content_stats and avg_value_size_in_bytes.
Generated-by: Cursor
Match other optional ContentFileParser fields: explicit JSON null is missing, and invalid field IDs get a contextual error.
Generated-by: Cursor
Satisfy checkstyle by creating the parsed content stats map with the relocated Maps utility.
Generated-by: Cursor
@huan233usc
huan233usc marked this pull request as ready for review September 1, 2026 18:25
@huan233usc

Copy link
Copy Markdown
ContributorAuthor

Updated the PR, PTAL @singhpk234@nastra@szehon-ho Thanks

content_stats: dict[str, FieldStatistics] | None = Field(
None,
alias='content-stats',
description='Container struct for per-field metrics structs',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could we document that content-stats keys are decimal string field IDs, ideally with an example? The parser requires numeric keys, while the OpenAPI schema currently allows arbitrary strings. Fine to address in a follow-up.

@szehon-ho
szehon-ho merged commit fa22021 into apache:mainSep 4, 2026
41 of 42 checks passed
@szehon-ho

Copy link
Copy Markdown
Member

Merged, thanks @huan233usc and @singhpk234 for additional early review !

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@huan233usc@szehon-ho@singhpk234