Skip to content

Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown #3284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kbendick wants to merge 4 commits into from
Closed

Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown #3284

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a204f31
Spec - Add -1 to data_file.record_count to indicate unknown record co…
kbendick Oct 12, 2021
4924662
Remove added period
kbendick Oct 12, 2021
fd501ab
Move -1 to be in description and match the language of row-oriented f…
kbendick Oct 12, 2021
20b9cc3
Simply mention that -1 is unknown without mentioning details
kbendick Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion site/docs/spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ The schema of a manifest file is a struct called `manifest_entry` with the follo
| _required_ | _required_ | **`100 file_path`** | `string` | Full URI for the file with FS scheme |
| _required_ | _required_ | **`101 file_format`** | `string` | String file format name, avro, orc or parquet |
| _required_ | _required_ | **`102 partition`** | `struct<...>` | Partition data tuple, schema based on the partition spec output using partition field ids for the struct field ids |
| _required_ | _required_ | **`103 record_count`** | `long` | Number of records in this file |
| _required_ | _required_ | **`103 record_count`** | `long` with special value: `-1: Record count unknown` | Number of records in this file. |

@szehon-ho szehon-ho Oct 12, 2021

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.

Shouldn't it be added to the last column? (description)

@kbendick kbendick Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I did that initially, but there are a few places that place it under description.

However, those are arguably placed under Type and not Description because they're enums, whereas this is just one possible magic value (so I personally agree that description is the better column for this).

That's mostly why I made this a draft actually 😅

Here's an example of the values being in Type:

iceberg/site/docs/spec.md

Line 488 in 86350db

| | _required_ | **`517 content`** | `int` with meaning: `0: data`, `1: deletes` | The type of files tracked by the manifest, either data or delete files; 0 for all v1 manifests |

As well as here:

iceberg/site/docs/spec.md

Lines 374 to 378 in 86350db

`data_file` is a struct with the following fields:
| v1 | v2 | Field id, name | Type | Description |
| ---------- | ---------- |-----------------------------------|------------------------------|-------------|
| | _required_ | **`134 content`** | `int` with meaning: `0: DATA`, `1: POSITION DELETES`, `2: EQUALITY DELETES` | Type of content stored by the data file: data, equality deletes, or position deletes (all v1 files are data files) |

@kbendick kbendick Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But since this is just a caveat on one magic value, I do tend to agree with you.

I'm open to either. Given it's the spec, I figured I'd let others weigh in.

@kbendick kbendick Oct 12, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated it to be in Description @szehon-ho. I copied language similar to an entry a few lines down.

| _required_ | _required_ | **`104 file_size_in_bytes`** | `long` | Total file size in bytes |
| _required_ | | ~~**`105 block_size_in_bytes`**~~ | `long` | **Deprecated. Always write a default in v1. Do not write in v2.** |
| _optional_ | | ~~**`106 file_ordinal`**~~ | `int` | **Deprecated. Do not write.** |
Expand Down