Re: [PR] fix(core): gate the log-block scan on instant state [hudi-rs]
yihua merged PR #678: URL: https://github.com/apache/hudi-rs/pull/678 -- 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]
Re: [PR] fix(core): gate the log-block scan on instant state [hudi-rs]
yihua commented on code in PR #678:
URL: https://github.com/apache/hudi-rs/pull/678#discussion_r3909575849
##
crates/core/tests/gold_parity_tests.rs:
##
@@ -502,6 +504,26 @@ impl Known {
/// disagreement fails the build, and one that starts passing has to be removed
/// from here, so this list cannot quietly go stale.
const KNOWN: &[Known] = &[
+// Version 1 only: it applies no completed/inflight check to log blocks, so
+// the blocks of a delta commit that never completed still reach the merge.
+// The fixture exists to pin that version 2 does check; this records that
+// version 1 does not, which is the divergence itself rather than a fixture
+// flaw. Only the version 6 fixture separates the two readers: on version 9
+// the log file is dropped when the slice is built, which both of them
share.
+//
+// Unlike the other version 1 entries here, which record a narrower or
+// differently-shaped answer, this one records a wrong one: rows that were
+// never committed. It is accepted rather than fixed because version 2 is
the
+// default and version 1 is on its way out. Two things would reopen that: a
+// version 1 read reachable without asking for it, or version 1 outliving
the
+// migration. Fixing it means giving `LogFileScanner` the instant state its
+// `scan` has no notion of, which reaches the metadata table reader too.
+Known {
+fixture: "table_uncommitted_log_v6",
+scope: CaseScope::Any,
+reader_version: "1",
+reason: "version 1 admits blocks from an instant that never completed",
+},
Review Comment:
non-blocking: `CaseScope::Any` here covers `key_only` and `read_optimized`,
which v1 passes on this fixture (the orphan is an update, so the key set is
unchanged, and read-optimized never touches the log). Per the `Any` scope's own
doc, would it be worth naming `SelectStar` plus the four cases that actually
fail, so a later v1 regression on the passing two still trips the ratchet?
--
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]
Re: [PR] fix(core): gate the log-block scan on instant state [hudi-rs]
codecov[bot] commented on PR #678: URL: https://github.com/apache/hudi-rs/pull/678#issuecomment-5309074288 ## [Codecov](https://app.codecov.io/gh/apache/hudi-rs/pull/678?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report :x: Patch coverage is `75.0%` with `9 lines` in your changes missing coverage. Please review. :white_check_mark: Project coverage is 80.32%. Comparing base ([`75b188e`](https://app.codecov.io/gh/apache/hudi-rs/commit/75b188ef2fbf7e3622c4424ff0d061c89be803b1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`9b01108`](https://app.codecov.io/gh/apache/hudi-rs/commit/9b011088851bb194fc91c00fb873bd32bd2e807b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)). | [Files with missing lines](https://app.codecov.io/gh/apache/hudi-rs/pull/678?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines | |---|---|---| | [crates/core/src/timeline/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftimeline%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RpbWVsaW5lL21vZC5ycw==) | 61.53% | [5 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [crates/core/src/table/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL21vZC5ycw==) | 76.92% | [3 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | | [crates/core/src/file\_group/reader.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ffile_group%2Freader.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL2ZpbGVfZ3JvdXAvcmVhZGVyLnJz) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/678?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Additional details and impacted files ```diff @@Coverage Diff @@ ## main #678 +/- ## == + Coverage 80.30% 80.32% +0.02% == Files 118 118 Lines 1104811077 +29 == + Hits 8872 8898 +26 - Misses 2176 2179 +3 ``` [:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/hudi-rs/pull/678?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). :rocket: New features to boost your workflow: - :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems. -- 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]
Re: [PR] fix(core): gate the log-block scan on instant state [hudi-rs]
linliu-code commented on PR #678: URL: https://github.com/apache/hudi-rs/pull/678#issuecomment-5302881311 Pushed `e2328bb`, a self-review catch on the first commit. Wiring the gate to `Table` made its own docs wrong: they described the producer as "the FFI bridge when the planner enabled the gate" and scoped the gate to "table version < 8 snapshot reads" — the reader-context comment yihua flagged on #660 ([reader_context.rs:126](https://github.com/apache/hudi-rs/pull/660#discussion_r3787843187)), now in the one PR that was meant to resolve it. The version scoping also stopped matching the code. The wiring populates the gate at both reader-construction sites unconditionally, so it applies to incremental reads too. That is intended, not an oversight: Gate 3 only ever *subtracts*, so it cannot admit a block Gate 4 rejected, and a pending instant has no business in an incremental window either. That was an argument, so it is now a test — `test_pass1_gate3_and_the_incremental_window_only_ever_subtract` pins both directions (pending-inside-window excluded, committed-outside-window still excluded). Mutation-checked by implementing the alternative — skip the gate when a range is set — under which it is the **only** failing test in the suite, so nothing else was pinning this. -- 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]
