You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/12 17:54:51 UTC
[GitHub] [iceberg] kbendick opened a new pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
kbendick opened a new pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284
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:
https://github.com/apache/iceberg/blob/7aef02b9a192f62740f16a883a6ec955cdcd7aff/api/src/main/java/org/apache/iceberg/expressions/InclusiveMetricsEvaluator.java#L90-L99
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick edited a comment on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick edited a comment on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-941239952
This is in reference to https://github.com/apache/iceberg/pull/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
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727460195
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
I updated it to be in `Description` @szehon-ho. I copied language similar to an entry a few lines down.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727452321
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
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.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-941239952
This is in reference to https://github.com/apache/iceberg/pull/3273, but the base implementation also does this and thus we should likely add this to the spec.
cc @RussellSpitzer @szehon-ho @aokolnychyi @rdblue
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue closed pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
rdblue closed pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727450562
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
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`:
https://github.com/apache/iceberg/blob/86350db9507d6b0d19643beae25de0a758b6d3cb/site/docs/spec.md#L488
As well as here:
https://github.com/apache/iceberg/blob/86350db9507d6b0d19643beae25de0a758b6d3cb/site/docs/spec.md#L374-L378
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
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.
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
I updated it to be in `Description` @szehon-ho. I copied language similar to an entry a few lines down.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727418411
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
Shouldn't it be added to the last column? (description)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-941239952
This is in reference to https://github.com/apache/iceberg/pull/3273, but the base implementation also does this and thus we should likely add this to the spec.
cc @RussellSpitzer @szehon-ho @aokolnychyi @rdblue
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick edited a comment on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick edited a comment on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-941239952
This is in reference to https://github.com/apache/iceberg/pull/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
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szehon-ho commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727418411
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
Shouldn't it be added to the last column? (description)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-942606758
> 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: https://github.com/apache/iceberg/blob/86350db9507d6b0d19643beae25de0a758b6d3cb/site/docs/spec.md#L397-L403
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.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-947918754
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
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#discussion_r727450562
##########
File path: site/docs/spec.md
##########
@@ -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. |
Review comment:
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`:
https://github.com/apache/iceberg/blob/86350db9507d6b0d19643beae25de0a758b6d3cb/site/docs/spec.md#L488
As well as here:
https://github.com/apache/iceberg/blob/86350db9507d6b0d19643beae25de0a758b6d3cb/site/docs/spec.md#L374-L378
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3284: Spec - Add -1 to Manifest Entry's data_file.record_count to indicate unknown
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3284:
URL: https://github.com/apache/iceberg/pull/3284#issuecomment-947928954
Yeah, I think that fixing the Avro bug was the important thing. Let's close this. Thanks @kbendick!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org