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

[kbendick]

In some cases, we don't know the count of records in a data file when we write the manifest entry.

This can happen particularly when importing files from Avro tables, as we would otherwise need to scan the entire file due to lack of built-in metrics to Avro files.

As we haven't implemented parsing record count from avro files, we set record count to -1 to indicate to the metrics evaluators that the file has rows which might match.

See an example here:

iceberg/api/src/main/java/org/apache/iceberg/expressions/InclusiveMetricsEvaluator.java

Lines 90 to 99 in 7aef02b

	if (file.recordCount() == 0) {
	return ROWS_CANNOT_MATCH;
	}
	if (file.recordCount() < 0) {
	// we haven't implemented parsing record count from avro file and thus set record count -1
	// when importing avro tables to iceberg tables. This should be updated once we implemented
	// and set correct record count.
	return ROWS_MIGHT_MATCH;
	}

[kbendick]

This is in reference to #3273, but the base implementation also does this and thus we should likely add this to the spec.

Note - Marking this as a draft as we might want to also call out that 0 indicates we should skip the file entirely (though that's perhaps overkill). Also, marking as draft as I'm open to formatting this differently.

cc @RussellSpitzer @szehon-ho @aokolnychyi @rdblue

[szehon-ho]

Makes sense to me to add -1 to spec (as @kbendick mentioned, my change is not the first to set it to this value)

[szehon-ho]

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

[kbendick]

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]

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]

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

[szehon-ho]

Looks good to me, only style thing is I am not sure if we need the example here as it gets a bit wordy (and might be a bit too specific to a spark-operation, for a generic spec)

[kbendick]

Looks good to me, only style thing is I am not sure if we need the example here as it gets a bit wordy (and might be a bit too specific to a spark-operation, for a generic spec)

I thought that too. One thing that exists is number marks that drop to a sentence with further detail, where necessary. Like [1].

Here's an example:

iceberg/site/docs/spec.md

Lines 397 to 403 in 86350db

	| _optional_ | _optional_ | **`140 sort_order_id`** | `int` | ID representing sort order for this file [3]. |
	Notes:
	1. Single-value serialization for lower and upper bounds is detailed in Appendix D.
	2. For `float` and `double`, the value `-0.0` must precede `+0.0`, as in the IEEE 754 `totalOrder` predicate.
	3. If sort order ID is missing or unknown, then the order is assumed to be unsorted. Only data files and equality delete files should be written with a non-null order id. [Position deletes](#position-delete-files) are required to be sorted by file and position, not a table order, and should set sort order id to null. Readers must ignore sort order id for position delete files.

However, I agree this one reads very particular to this case.

My desire was to indicate that -1 shouldn't be common, under normal situations. Though I'm not sure how true that is or if it matters.

Will update to be more generic for now.

[szehon-ho]

Thanks @kbendick looks good from my end

[kbendick]

@rdblue are you ok with this minor update to the spec? It reflects current behavior, even in the base classes (mentioned above).

I had considered adding a footnote that this is a rare occurrence (i.e. it shouldn't be common or something people expect during normal operations), but seemed possibly too much information for the spec.

[kbendick]

Based on this PR, is this no longer needed (at least for the Avro import path)? https://github.com/apache/iceberg/pull/3273/files @szehon-ho

[rdblue]

Yeah, I think that fixing the Avro bug was the important thing. Let's close this. Thanks @kbendick!