-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Output metrics about remove_orphan_files execution #26661
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the Iceberg connector's remove_orphan_files
procedure to output execution metrics. Instead of returning no information, the procedure now returns a table with metrics including manifest file processing count, valid files found, files scanned, and files deleted.
- Modified the
executeTableExecute
method signature across the SPI to returnMap<String, Long>
instead ofvoid
- Enhanced the
remove_orphan_files
implementation to collect and return execution metrics - Updated query result handling to display metrics as a two-column table with metric names and values
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java | Changed executeTableExecute return type from void to Map<String, Long> |
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java | Implemented metrics collection and return for remove_orphan_files procedure |
core/trino-main/src/main/java/io/trino/operator/SimpleTableExecuteOperator.java | Updated operator to build result pages from returned metrics |
core/trino-main/src/main/java/io/trino/sql/planner/plan/SimpleTableExecuteNode.java | Changed from single output symbol to list of symbols for metric name/value columns |
docs/src/main/sphinx/connector/iceberg.md | Added documentation for the new metrics output format |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java
Outdated
Show resolved
Hide resolved
3577a9d
to
b63db28
Compare
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Outdated
Show resolved
Hide resolved
List<MaterializedRow> expectedRows = expectedResults.getMaterializedRows(); | ||
|
||
if (compareUpdate) { | ||
if (compareUpdate && !actualResults.getUpdateType().equals(Optional.of("ALTER TABLE EXECUTE"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of skipping here, can we introduce something assertAlter
or a dedicated method - which would set compareUpdate
to false ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no need for a special assertAlter
method. We should be able to assert a more detailed output with assertUpdate itself.
This branch is written with the assumption that an update query can only produce a row count output and nothing else. Since ALTER commands did not output anything before, this branch was never executed before.
Now with the more detailed output, we need to skip over this branch to allow the assertion on detailed results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add boolean compareExecute
boolean flag and special assertExecute
method?
c75a156
to
0a72e6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % comments
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java
Show resolved
Hide resolved
List<MaterializedRow> expectedRows = expectedResults.getMaterializedRows(); | ||
|
||
if (compareUpdate) { | ||
if (compareUpdate && !actualResults.getUpdateType().equals(Optional.of("ALTER TABLE EXECUTE"))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add boolean compareExecute
boolean flag and special assertExecute
method?
0a72e6b
to
c8e5c41
Compare
c8e5c41
to
41883de
Compare
alter table lineitem execute remove_orphan_files(retention_threshold => '0d'); metric_name | metric_value ----------------------------+-------------- processed_manifests_count | 2 active_files_count | 98 scanned_files_count | 97 deleted_files_count | 0
41883de
to
420b8ed
Compare
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: