[core] Spell out what a negative partition statistic means - #9120
Open
sundapeng wants to merge 1 commit into
Open
[core] Spell out what a negative partition statistic means#9120sundapeng wants to merge 1 commit into
sundapeng wants to merge 1 commit into
Conversation
PartitionStatistics said only that its fields "may be negative, indicating that some data has been removed". That covers one of the two planes the class is read on, and consumers have been getting the other one wrong. On the delta plane — what a commit changed — a negative value is a decrement the server adds to what it holds. That is the existing meaning and nothing here changes it. On the observation plane — what listPartitions returns for a partition as it stands — a negative value means nobody ever reported that field, and 0 means an exact zero. Conflating them is not cosmetic: a consumer that reads unknown as zero plans against an empty partition that may hold a billion rows, and one that does arithmetic on it gets a number that is wrong rather than missing. So the plane is named in the javadoc, unknown gets a name (UNKNOWN, with isKnown() to test it rather than each caller comparing against -1), and unknown is documented as per field: a reporter that only knows the file count leaves the record count unknown and fills the rest. The fields stay primitive. Boxing them to express unknown as null would be a breaking change to a @public class, and the encoding above needs no new type. FileSystemSplitEnumerator now says PartitionStatistics.UNKNOWN where it said -1. Discovering partitions by listing directories measures nothing about what is inside them, which is what unknown already meant there; this is the same value under its own name.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
PartitionStatisticssays only that its fields "may be negative, indicating that some data has beenremoved". That covers one of the two planes the class is read on, and consumers have been getting the
other one wrong.
holds. That is the existing meaning and nothing here changes it.
listPartitionsreturns for a partition as it stands. A negative valuemeans nobody ever reported that field, and
0means an exact zero.Conflating them is not cosmetic. A consumer that reads unknown as zero plans against an empty
partition that may hold a billion rows; one that does arithmetic on it gets a number that is wrong
rather than missing. This has already produced a silent wrong answer in a sibling project, where
COUNT(*)was answered from a placeholder row count and returned zero for a partition full of data(apache/paimon-rust#624).
So: the plane is named in the javadoc, unknown gets a name (
UNKNOWN, withisKnown()to test itrather than each caller comparing against
-1), and unknown is documented as per field — areporter that only knows the file count leaves the record count unknown and fills the rest.
Why the fields stay primitive
Boxing them to express unknown as
nullwould be a breaking change to a@Publicclass, and theencoding above needs no new type.
Behaviour
Zero change.
UNKNOWNis a name for a value already in use.FileSystemSplitEnumeratornow saysPartitionStatistics.UNKNOWNwhere it said-1— the same value under its own name, becausediscovering partitions by listing directories measures nothing about what is inside them, which is
what unknown already meant there.
Context
This is the first of a stack that lets a catalog-managed format table report partition statistics; a
format table has no snapshot, so the channel a table snapshot uses does not exist for it. This PR is
worth having on its own regardless of the rest of that stack: the ambiguity it removes has already
cost one silent wrong answer.
API and Format
PartitionStatisticsgainsUNKNOWN,UNKNOWN_TOTAL_BUCKETS,unknown(spec)andisKnown(long). Additive only; no existing signature changes. No format change.Documentation
The contract now lives in the class javadoc, which is where a consumer looks.