Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add API changes for statistics information in table metadata #5021

Merged
merged 1 commit into from
Aug 30, 2022
Merged

Add API changes for statistics information in table metadata #5021

merged 1 commit into from
Aug 30, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Jun 13, 2022

API for #4945
Implementation PR: #4741

Extracted from #4741 per #4741 (comment)

@findepi findepi mentioned this pull request Jun 13, 2022
Merged
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from a593574 to b37e321 Compare June 14, 2022 08:04
@findepi
Copy link
Member Author

findepi commented Jun 14, 2022

@rdblue thank you for your review. Updated accordingly.

@findepi findepi requested a review from rdblue June 14, 2022 08:06
rdblue
rdblue reviewed Jun 15, 2022
View reviewed changes
core/src/main/java/org/apache/iceberg/BaseMetadataTable.java
@@ -216,6 +216,11 @@ public DeleteFiles newDelete() {
throw new UnsupportedOperationException("Cannot delete from a metadata table");
}

@Override
public UpdateTableStatistics updateTableStatistics() {
throw new UnsupportedOperationException("Cannot update statistics of a metadata table");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add this as a default implementation rather than adding it in this class? That way, if anyone is using the interface it doesn't break them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, added default method and removed impl stubs from BaseTable and BaseTransaction.
Impl in BaseMetadataTable stays because it includes "a metadata table" in the exc msg (consistently with all other methods in this class).

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from b37e321 to 74bd319 Compare June 20, 2022 07:52
@findepi findepi requested a review from rdblue June 20, 2022 07:56
@findepi
Copy link
Member Author

findepi commented Jun 20, 2022

Currently, based on #4978

@github-actions github-actions bot added the build label Jun 20, 2022
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 2 times, most recently from 9791c86 to 92e88c4 Compare June 22, 2022 19:44
@findepi
Copy link
Member Author

findepi commented Jun 22, 2022

@rdblue updated this to have one stats file for a table.

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from 92e88c4 to f5e3f8f Compare June 22, 2022 20:12
This was referenced Jun 23, 2022
Merged
Merged
@findepi
Copy link
Member Author

findepi commented Jun 24, 2022

Per offline ask from @rdblue , I extracted prep change here: #5129

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from f5e3f8f to 3c1e5c6 Compare June 27, 2022 13:19
@findepi findepi changed the title Add API changes for statistics information in table snapshot Add API changes for statistics information in table metadata Aug 1, 2022
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 3 times, most recently from 42b0dfa to 0f451b8 Compare August 1, 2022 15:51
@findepi
Copy link
Member Author

findepi commented Aug 1, 2022

Updated after merge of #4945.

@rdblue can you please take a look?

@findepi findepi removed the request for review from rdblue August 1, 2022 15:54
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 2 times, most recently from 22d6e91 to b899566 Compare August 5, 2022 18:25
@findepi findepi mentioned this pull request Aug 5, 2022
Merged
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from b899566 to 07418e2 Compare August 5, 2022 18:28
@findepi
Copy link
Member Author

findepi commented Aug 5, 2022

@rdblue thank you for a careful review. Comments applied, the code is much better now.
The potentially immature changes to TableMetadata are moved to #5450

Can you please take another look?

@findepi findepi requested a review from rdblue August 5, 2022 18:29
@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from 07418e2 to 0c9eb05 Compare August 8, 2022 09:50
@findepi
Copy link
Member Author

findepi commented Aug 8, 2022

Updated to apply #5450 (comment).

@findepi
Copy link
Member Author

findepi commented Aug 9, 2022

Build failure

> Could not resolve all files for configuration ':classpath'.
   > Could not resolve com.palantir.baseline:gradle-baseline-java:4.0.0.
     Required by:
         project :
      > Could not resolve com.palantir.baseline:gradle-baseline-java:4.0.0.
         > Could not get resource 'https://plugins.gradle.org/m2/com/palantir/baseline/gradle-baseline-java/4.0.0/gradle-baseline-java-4.0.0.pom'.
            > Could not GET 'https://plugins.gradle.org/m2/com/palantir/baseline/gradle-baseline-java/4.0.0/gradle-baseline-java-4.0.0.pom'.
               > The server may not support the client's requested TLS protocol versions: (TLSv1.2, TLSv1.3). You may need to configure the client to allow other protocols to be used. See: https://docs.gradle.org/7.4.2/userguide/build_environment.html#gradle_system_properties
                  > Received fatal alert: handshake_failure

looks unrelated.

Let me push amended commit to restart the build.

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 2 times, most recently from 1bba34e to 21f012f Compare August 11, 2022 13:33
@findepi findepi mentioned this pull request Aug 12, 2022
Closed
@findepi
Copy link
Member Author

findepi commented Aug 23, 2022

Currently, depends on #5450

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from 21f012f to 20d6c0d Compare August 25, 2022 11:20
@findepi
Copy link
Member Author

findepi commented Aug 25, 2022

Currently, depends on #5450

rebased after that one is merged.

@rdblue please take a look

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 2 times, most recently from 3f50ce0 to 1c66ccb Compare August 25, 2022 19:54
rdblue
rdblue reviewed Aug 26, 2022
View reviewed changes
api/src/main/java/org/apache/iceberg/UpdateStatistics.java Outdated
*
* @return this for method chaining
*/
UpdateStatistics setStatisticsFile(long snapshotId, @Nullable StatisticsFile statisticsFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove the @Nullable annotation?

How about naming this setStatistics like in TableMetadata.Builder?

Also, I thought we needed a removeStatistics method as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@rdblue
Copy link
Contributor

rdblue commented Aug 26, 2022

@findepi, this is looking close. I think we just need to finalize the UpdateStatistics methods. Do you have a PR for the UpdateStatistics implementation? We should be able to work on that in parallel.

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch 2 times, most recently from 91d98c6 to 6d5c136 Compare August 27, 2022 21:24
@findepi
Copy link
Member Author

findepi commented Aug 27, 2022

I have applied the requested changes. @rdblue please take another look

@findepi findepi force-pushed the findepi/stats-in-table-just-api branch from 6d5c136 to 81706c8 Compare August 29, 2022 15:01
@rdblue rdblue merged commit cc7e6d9 into apache:master Aug 30, 2022
@findepi findepi deleted the findepi/stats-in-table-just-api branch August 30, 2022 17:37
@findepi
Copy link
Member Author

findepi commented Aug 30, 2022

Thank you for the merge!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdblue rdblue rdblue approved these changes

@danielcweeks danielcweeks Awaiting requested review from danielcweeks

Assignees
No one assigned
Labels
API build core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@findepi @rdblue