Skip to content

Docs: Update spec about statistics file snapshot id - #6267

Merged
rdblue merged 1 commit into
apache:masterfrom
ajantha-bhat:stats
Dec 19, 2022
Merged

Docs: Update spec about statistics file snapshot id#6267
rdblue merged 1 commit into
apache:masterfrom
ajantha-bhat:stats

Conversation

@ajantha-bhat

@ajantha-bhatajantha-bhat commented Nov 24, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Background:

Spec has a snapshot id in two places, one in StatisticsFile and another in its blob metadata.
To support the reuse of statistics files, we should have the referenced snapshot id in StatisticsFile, not the computed-from snapshot id. Hence, updated the spec.

Note that PR #6090 is stuck because of confusion around stats file reuse.

@ajantha-bhat
ajantha-bhat marked this pull request as draft November 24, 2022 17:27
@ajantha-bhat

ajantha-bhat commented Nov 24, 2022

Copy link
Copy Markdown
MemberAuthor

@rdblue, @findepi: Please let me know your thoughts on this.

ajantha-bhat
ajantha-bhat commented Nov 25, 2022
View reviewed changes
Comment threadformat/spec.md Outdated
@ajantha-bhat
ajantha-bhatforce-pushed the stats branch 4 times, most recently from 8537ce6 to 1e0906eCompareDecember 3, 2022 09:39
ajantha-bhat
ajantha-bhat commented Dec 3, 2022
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TableMetadata.java Outdated
private List<Snapshot> snapshots;
private final Map<String, SnapshotRef> refs;
private final Map<Long, List<StatisticsFile>> statisticsFiles;
private final Map<Long, List<StatisticsFile>> statisticsFilesById;

@ajantha-bhatajantha-bhatDec 3, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought better to rename this similar to snapshotsById

ajantha-bhat
ajantha-bhat commented Dec 3, 2022
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TableMetadata.java
"snapshotId does not match: %s vs %s",
snapshotId,
statisticsFile.snapshotId());
statisticsFiles.put(statisticsFile.snapshotId(), ImmutableList.of(statisticsFile));

@ajantha-bhatajantha-bhatDec 3, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interface supports returning multiple stats file for one snapshot. But was always overwriting instead of appending. So, fixed it.

@rdbluerdblueDec 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not correct. There should be only one stats file for a snapshot that contains all information. Allowing multiple stats files for a snapshot is going to lead to lazy implementations that don't merge the files and slower job planning.

@ajantha-bhatajantha-bhatDec 9, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rdblue, @findepi:
It is just as per the previous existing interface in the builder.
we had Map<Long, List<StatisticsFile>> statisticsFiles in builder.
If we wanted one stats file per interface then no need to have list for map value?

Also is it possible that there will be one partition level stats (avro/parquet file) and there is one table level NDV file per snapshot. In this case, no need to merge them but can be used based on the need. So, still ok to have multiple stats per snapshot id?

ajantha-bhat
ajantha-bhat commented Dec 3, 2022
View reviewed changes
Comment threadcore/src/test/java/org/apache/iceberg/TestTableMetadata.java Outdated
Assert.assertEquals("Statistics file path", "/some/path/to/stats/file2", statisticsFile.path());
Assertions.assertThat(withStatisticsAppended.statisticsFiles())
.as("There should be two statistics files registered")
.hasSize(2)

@ajantha-bhatajantha-bhatDec 3, 2022

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I fixed the overwrite to append. There will be two stats file for this snapshot id. Hence, updated the testcases.

@rdbluerdblueDec 8, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is incorrect.

@ajantha-bhat
ajantha-bhat marked this pull request as ready for review December 3, 2022 10:34
@ajantha-bhat

ajantha-bhat commented Dec 3, 2022

Copy link
Copy Markdown
MemberAuthor

@rdblue, @findepi: PR is ready for review. This should help in unblocking #6090, #6091

This was referenced Dec 5, 2022
Merged
Merged
rdblue
rdblue reviewed Dec 8, 2022
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TableMetadata.java Outdated
rdblue
rdblue reviewed Dec 8, 2022
View reviewed changes
Comment threadcore/src/main/java/org/apache/iceberg/TableMetadata.java Outdated
@rdblue

rdblue commented Dec 8, 2022

Copy link
Copy Markdown
Contributor

@ajantha-bhat, I'm fine with the update to the spec wording, but I don't think the rest of these changes are needed. There should be only one stats file per snapshot in metadata. There is no value in having multiple stats files and implementations should merge them.

@ajantha-bhat

ajantha-bhat commented Dec 9, 2022

Copy link
Copy Markdown
MemberAuthor

@ajantha-bhat, I'm fine with the update to the spec wording, but I don't think the rest of these changes are needed.

I think we still need an interface to get the current stats file for snapshot id currentStatisticsFiles()

There should be only one stats file per snapshot in metadata.
There is no value in having multiple stats files and implementations should merge them.

Maybe true. But It was just as per the previous existing interface in the builder.
we had Map<Long, List<StatisticsFile>> statisticsFiles in builder.
If we wanted one stats file per interface then no need to have list for map value?

Also is it possible that there will be one partition level stats (avro/parquet file) and there is one table level NDV file per snapshot. In this case, no need to merge them but can be used based on the need. So, still ok to have multiple stats per snapshot id?

@rdblue

rdblue commented Dec 12, 2022

Copy link
Copy Markdown
Contributor

I think we still need an interface to get the current stats file for snapshot id currentStatisticsFiles()

No, I don't think this is valuable. This depends too much on some definition of "current", which is about as useful as asking for any stats file. This should be more specific. I agree that we want some way to get a stats file, but I don't think there's much value in the "current" idea.

It was just as per the previous existing interface in the builder.

I wouldn't worry too much about intermediate representations. What we have in the current API is fine for now. I don't think that we should make the changes in this PR.

@ajantha-bhatajantha-bhat changed the title Core: Update StatisticsFile interface in TableMetadata specDocs: Update spec about statistics file snapshot idDec 12, 2022
@ajantha-bhat

ajantha-bhat commented Dec 12, 2022

Copy link
Copy Markdown
MemberAuthor

@rdblue: Thanks for the review and suggestions. I have kept it as just the document (spec) update now.

@rdblue

rdblue commented Dec 19, 2022

Copy link
Copy Markdown
Contributor

Thanks, @ajantha-bhat! Merging this.

@rdblue
rdblue merged commit bd526a1 into apache:masterDec 19, 2022
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepifindepifindepi left review comments
@rdbluerdbluerdblue approved these changes

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ajantha-bhat@rdblue@findepi