malinjawi commented on PR #12215: URL: https://github.com/apache/gluten/pull/12215#issuecomment-5425530571
> #12846 with updated Velox is merged @felipepessoto thanks for tracking all this down the scan reader root cause, https://github.com/facebookincubator/velox/pull/18536, and the #12815 / #12051 attribution. Saved me a lot of guessing. @zhztheplayer https://github.com/facebookincubator/velox/pull/18536 is in main now via [#12846](https://github.com/apache/incubator-gluten/issues/12846), so we can just test this instead of arguing it. I haven't run the DV suites with the flag on yet so I can't say it's clean. Let me flip the default on a scratch commit and run them with Velox output validation on. 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. 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. -- 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]
