aokolnychyi commented on code in PR #7105:
URL: https://github.com/apache/iceberg/pull/7105#discussion_r1351007189


##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 
+it must be registered in the table metadata file to be considered as a valid 
statistics file for the reader.
+
+`partition-statistics` field of table metadata is an optional list of struct 
with the following fields:
+
+| v1 | v2 | Field name | Type | Description |
+|----|----|------------|------|-------------|
+| _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg 
table's snapshot the partition statistics file is associated with. |
+| _required_ | _required_ | **`statistics-file-path`** | `string` | Path of 
the partition statistics file. See [Partition Statistics 
file](#partition-statistics-file). |
+| _required_ | _required_ | **`file-size-in-bytes`** | `long` | Size of the 
partition statistics file. |
+
+#### Partition Statistics file

Review Comment:
   Question: Shall `file` be `File` since it is a header?



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 

Review Comment:
   Question: Is there a particular reason to use capital letters for `Partition 
Statistics`? It seems inconsistent with other places.



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 
+it must be registered in the table metadata file to be considered as a valid 
statistics file for the reader.
+
+`partition-statistics` field of table metadata is an optional list of struct 
with the following fields:
+
+| v1 | v2 | Field name | Type | Description |
+|----|----|------------|------|-------------|
+| _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg 
table's snapshot the partition statistics file is associated with. |
+| _required_ | _required_ | **`statistics-file-path`** | `string` | Path of 
the partition statistics file. See [Partition Statistics 
file](#partition-statistics-file). |

Review Comment:
   I see your point but isn't it also a path to a single file in case of table 
stats? I would align the naming to be consistent if it indicates a path to a 
single file in both cases.



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 

Review Comment:
   Shall we add a note that it can be also computed on demand rather that in 
each write?



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 
+it must be registered in the table metadata file to be considered as a valid 
statistics file for the reader.
+
+`partition-statistics` field of table metadata is an optional list of struct 
with the following fields:
+
+| v1 | v2 | Field name | Type | Description |
+|----|----|------------|------|-------------|
+| _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg 
table's snapshot the partition statistics file is associated with. |
+| _required_ | _required_ | **`statistics-file-path`** | `string` | Path of 
the partition statistics file. See [Partition Statistics 
file](#partition-statistics-file). |
+| _required_ | _required_ | **`file-size-in-bytes`** | `long` | Size of the 
partition statistics file. |
+
+#### Partition Statistics file
+
+Statistics information for each unique partition tuple is stored as a row in 
the default data file format of the table (for example, Parquet or ORC).
+These rows must be sorted (in ascending manner with NULL FIRST) by `partition` 
field to optimize filtering rows while scanning.
+
+The schema of the partition statistics file is as follows:
+
+| v1 | v2 | Field id, name | Type | Description |
+|----|----|----------------|------|-------------|
+| _required_ | _required_ | **`1 partition`** | `struct<..>` | Partition data 
tuple, schema based on the unified partition type considering all specs in a 
table |
+| _required_ | _required_ | **`2 spec_id`** | `int` | Partition spec id |
+| _required_ | _required_ | **`3 data_record_count`** | `long` | Count of 
records in data files |
+| _required_ | _required_ | **`4 data_file_count`** | `int` | Count of data 
files |
+| _required_ | _required_ | **`5 total_data_file_size_in_bytes`** | `long` | 
Total size of data files in bytes |
+| _optional_ | _optional_ | **`6 position_delete_record_count`** | `long` | 
Count of records in position delete files |
+| _optional_ | _optional_ | **`7 position_delete_file_count`** | `int` | Count 
of position delete files |
+| _optional_ | _optional_ | **`8 equality_delete_record_count`** | `long` | 
Count of records in equality delete files |
+| _optional_ | _optional_ | **`9 equality_delete_file_count`** | `int` | Count 
of equality delete files |
+| _optional_ | _optional_ | **`10 total_record_count`** | `long` | Accurate 
count of records in a partition after applying the delete files if any |

Review Comment:
   Am I right this would only be possible to compute by reading data and 
applying deletes? If so, are we planning to make this optional and not populate 
by default?



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 
+it must be registered in the table metadata file to be considered as a valid 
statistics file for the reader.
+
+`partition-statistics` field of table metadata is an optional list of struct 
with the following fields:
+
+| v1 | v2 | Field name | Type | Description |

Review Comment:
   This seems reasonable, we can add it later.



##########
format/spec.md:
##########
@@ -702,6 +703,58 @@ Blob metadata is a struct with the following fields:
 | _optional_ | _optional_ | **`properties`** | `map<string, string>` | 
Additional properties associated with the statistic. Subset of Blob properties 
in the Puffin file. |
 
 
+#### Partition statistics
+
+Partition statistics files are based on [Partition Statistics file 
spec](#partition-statistics-file). 
+Partition statistics are not required for reading or planning and readers may 
ignore them.
+Each table snapshot may be associated with at most one partition statistic 
file.
+A writer can optionally write the partition statistics file during each write 
operation, and 
+it must be registered in the table metadata file to be considered as a valid 
statistics file for the reader.
+
+`partition-statistics` field of table metadata is an optional list of struct 
with the following fields:
+
+| v1 | v2 | Field name | Type | Description |
+|----|----|------------|------|-------------|
+| _required_ | _required_ | **`snapshot-id`** | `long` | ID of the Iceberg 
table's snapshot the partition statistics file is associated with. |
+| _required_ | _required_ | **`statistics-file-path`** | `string` | Path of 
the partition statistics file. See [Partition Statistics 
file](#partition-statistics-file). |
+| _required_ | _required_ | **`file-size-in-bytes`** | `long` | Size of the 
partition statistics file. |
+
+#### Partition Statistics file
+
+Statistics information for each unique partition tuple is stored as a row in 
the default data file format of the table (for example, Parquet or ORC).

Review Comment:
   I am not sure it is a good idea for the spec to make this assumption. I can 
see this being configurable even for tables that store Avro data to use Parquet 
or ORC for partition stats. Why can't we just default this in the 
implementation?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org

Reply via email to