moomindani commented on PR #16250: URL: https://github.com/apache/iceberg/pull/16250#issuecomment-5627881349
@nastra your eight threads here were all answered but never closed, so the PR reads as having eight outstanding items. I went through each one against the current head (`55c428ec5`) and resolved them. Please reopen any I got wrong — I would rather that than have the state misrepresent the PR. - Link to the ScanReport/CommitReport sections: anchors at `metrics-reporting.md:125`. - Wording suggestion: applied verbatim, line 137. - Empty string may be awkward from Spark, consider `none`: added. The code accepts `""` and `none` case-insensitively, covered by `testAttributesAllowlistNoneEmitsNoAttributes`. - Section goes beyond Iceberg's responsibility: the three sections are now one "Getting started", with Dependencies / Registering the SDK / Sending metrics to a backend. - Contradicts the table above, and the snapshot sentence is unclear: both gone; the section now ends by pointing back to Attribute set. - Commit metrics incomplete: all 24 accessors on `CommitMetricsResult` are mapped, manifests created/replaced/kept and manifest-entries-processed included. I verified this by extracting the interface's accessor names and diffing them against what `CommitInstruments` references, rather than by eye. - Scan metrics incomplete: the same check against `ScanMetricsResult`, all 16 mapped. - Wrapper instead of scan/commit prefixes: `ScanInstruments` and `CommitInstruments`. Correcting one number from my June reply: I said 28 commit instruments; it is 30. 45/45 green and mergeable — would you take another look? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
