You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2023/03/01 15:02:42 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #6975: Core: Minor refactoring of PartitionsTable
ajantha-bhat opened a new pull request, #6975:
URL: https://github.com/apache/iceberg/pull/6975
Changes:
- schema changes
- move spec_id (without changing field id) before the counters. Because It looks odd when we add new counters to have spec_id in between.
- add column docs for `record_count` and `file_count` to clarify that it is only data file counters.
- Rename internal fields `recordCount` to `dataRecordCount` and `fileCount` to `dataFileCount` for better clarity.
--
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] jackye1995 merged pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #6975:
URL: https://github.com/apache/iceberg/pull/6975
--
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] jackye1995 commented on a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1122630034
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -45,9 +45,14 @@ public class PartitionsTable extends BaseMetadataTable {
this.schema =
new Schema(
Types.NestedField.required(1, "partition", Partitioning.partitionType(table)),
- Types.NestedField.required(2, "record_count", Types.LongType.get()),
- Types.NestedField.required(3, "file_count", Types.IntegerType.get()),
- Types.NestedField.required(4, "spec_id", Types.IntegerType.get()));
+ Types.NestedField.required(4, "spec_id", Types.IntegerType.get()),
Review Comment:
I am not sure about the column order, it feels odd to me to have IDs in 1,4,2,3 order. But that's just personal preference.
--
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 a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1125757401
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -45,9 +45,11 @@ public class PartitionsTable extends BaseMetadataTable {
this.schema =
new Schema(
Types.NestedField.required(1, "partition", Partitioning.partitionType(table)),
- Types.NestedField.required(2, "record_count", Types.LongType.get()),
- Types.NestedField.required(3, "file_count", Types.IntegerType.get()),
- Types.NestedField.required(4, "spec_id", Types.IntegerType.get()));
+ Types.NestedField.required(4, "spec_id", Types.IntegerType.get()),
+ Types.NestedField.required(
+ 2, "record_count", Types.LongType.get(), "count of records in data files"),
+ Types.NestedField.required(
+ 3, "file_count", Types.IntegerType.get(), "count of data files"));
Review Comment:
The docs should use sentence case.
--
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 a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1125757566
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -77,7 +79,7 @@ private DataTask task(StaticTableScan scan) {
schema(),
scan.schema(),
partitions,
- root -> StaticDataTask.Row.of(root.recordCount, root.fileCount));
+ root -> StaticDataTask.Row.of(root.dataRecordCount, root.dataFileCount));
Review Comment:
I don't see much value in these renames, but it's okay with me.
--
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] ajantha-bhat commented on a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1122644862
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -45,9 +45,14 @@ public class PartitionsTable extends BaseMetadataTable {
this.schema =
new Schema(
Types.NestedField.required(1, "partition", Partitioning.partitionType(table)),
- Types.NestedField.required(2, "record_count", Types.LongType.get()),
- Types.NestedField.required(3, "file_count", Types.IntegerType.get()),
- Types.NestedField.required(4, "spec_id", Types.IntegerType.get()));
+ Types.NestedField.required(4, "spec_id", Types.IntegerType.get()),
Review Comment:
we have discussed this here
https://github.com/apache/iceberg/pull/6661#discussion_r1087858684
I think @RussellSpitzer and @szehon-ho agree that there is no impact from column re-ordering.
Spec-id in the middle really looks odd when we add other counters.
--
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] jackye1995 commented on pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#issuecomment-1477208246
Sorry overlooked this PR, looks like all comments are addressed, I will go ahead to merge it.
--
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] ajantha-bhat commented on pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#issuecomment-1455338420
@rdblue: Thanks for the review. I have fixed the case 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] ajantha-bhat commented on pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#issuecomment-1463739013
Hi, Can this PR be merged if no more comments or no new reviewers?
--
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] ajantha-bhat commented on a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1122644862
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -45,9 +45,14 @@ public class PartitionsTable extends BaseMetadataTable {
this.schema =
new Schema(
Types.NestedField.required(1, "partition", Partitioning.partitionType(table)),
- Types.NestedField.required(2, "record_count", Types.LongType.get()),
- Types.NestedField.required(3, "file_count", Types.IntegerType.get()),
- Types.NestedField.required(4, "spec_id", Types.IntegerType.get()));
+ Types.NestedField.required(4, "spec_id", Types.IntegerType.get()),
Review Comment:
we have discussed this here
https://github.com/apache/iceberg/pull/6661#discussion_r1087858684
I think @RussellSpitzer and @szehon-ho agree that there is no impact from column re-ordering.
Spec-id in the middle really looks odd when we add other counters. I don't want to change the field id and break the compatibility.
--
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] ajantha-bhat commented on pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#issuecomment-1450299741
cc: @szehon-ho, @aokolnychyi, @RussellSpitzer, @jackye1995
--
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 a diff in pull request #6975: Core: Minor refactoring of PartitionsTable
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #6975:
URL: https://github.com/apache/iceberg/pull/6975#discussion_r1125757483
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -332,7 +332,8 @@ public void testPartitionsTableScanWithProjection() {
Table partitionsTable = new PartitionsTable(table);
Types.StructType expected =
- new Schema(required(3, "file_count", Types.IntegerType.get())).asStruct();
+ new Schema(required(3, "file_count", Types.IntegerType.get(), "count of data files"))
Review Comment:
Docs should use sentence case here as well.
--
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