Core: Minor refactoring of PartitionsTable #6975
Conversation
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
Apart from the column order, I am good with the refactoring based on what we want to add next. Wondering what others think.
| 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()), |
There was a problem hiding this 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.
There was a problem hiding this comment.
we have discussed this here
#6661 (comment)
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.
| 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")); |
There was a problem hiding this comment.
The docs should use sentence case.
| 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")) |
There was a problem hiding this comment.
Docs should use sentence case here as well.
| scan.schema(), | ||
| partitions, | ||
| root -> StaticDataTask.Row.of(root.recordCount, root.fileCount)); | ||
| root -> StaticDataTask.Row.of(root.dataRecordCount, root.dataFileCount)); |
There was a problem hiding this comment.
I don't see much value in these renames, but it's okay with me.
|
@rdblue: Thanks for the review. I have fixed the case now. |
|
Hi, Can this PR be merged if no more comments or no new reviewers? |
|
Sorry overlooked this PR, looks like all comments are addressed, I will go ahead to merge it. |
3 participants
Changes:
record_countandfile_countto clarify that it is only data file counters.recordCounttodataRecordCountandfileCounttodataFileCountfor better clarity.This PR is a prerequisite for #6661