malinjawi commented on PR #12215: URL: https://github.com/apache/gluten/pull/12215#issuecomment-5438273030
> Hi @malinjawi @felipepessoto, thanks for keeping troubleshooting and for iterating the patch. > > > If nothing reproduces I'll delete the scan-shape detection and this PR becomes just the config + gate coverage, default on. You're right that disabling one scan type is a lot of machinery if the bug is gone. > > Yes. Regarding the change itself, what concerned me a little bit is the new tag introduced. Generally we need to minimize the number of cross-rule tags to limit the complexity of the query planner. So I would suggest to check whether the conditioned-fallback code can be put in one single place rather than tagging, if this PR needs to be continued further. > > > On the feature-level switch — agreed it's missing and it's the better knob either way. I'd do it as a follow-up so this PR stays one thing, but if you want it here I'll put it here and drop the scan-specific guard in the same change. > > Yes, we can open another PR for that. Thanks. @zhztheplayer done, this revision follows that exactly. The tag, the tagging rule and the post-transform subtree rule are gone (DeltaDeletionVectorDmlUtils deleted). The fallback check now lives in one place, OffloadDeltaScan, decided from the scan alone: the DML target's TahoeBatchFileIndex carries the command name and survives AQE stage splits, and only DV-writing DML reads a row-index column from it. enableNativeDmlRowIndexScan now defaults to true, and disabling it keeps only that one scan on Spark. On the feature-level DV switch: agreed, I'll open a separate PR after this lands. > @malinjawi FYI, I have tested after the daily velox update and debug.validate_output_from_operators enabled here: [#12783 (comment)](https://github.com/apache/gluten/pull/12783#issuecomment-5377218836) > > It doesn't show the CIs anymore, I guess this was the run: https://github.com/apache/gluten/actions/runs/32538314861 @felipepessoto thanks for the verification. For the record, run 32538314861 is red only on FAIL_ON_FIXED bookkeeping (two decimal type-widening tests that now pass after the Velox update) — 0 output-validation failures, 0 regressions, 0 quarantined DV errors across the suite. Since main has no guard, that run exercised the native DML target scan directly, which is what backs flipping the default here. One test note: the repeated-DELETE case now deletes leading rows first, so a scan that renumbered row indexes after applying an existing DV (the [#12377](https://github.com/apache/gluten/issues/12377) failure mode) would fail the cardinality and result checks instead of passing by luck. PTAL. -- 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]
