2010YOUY01 commented on PR #26227: URL: https://github.com/apache/datafusion/pull/26227#issuecomment-6227557496
Thank you for the review @andygrove Note this PR depends on https://github.com/apache/datafusion/pull/26410 -- would love to hear your judgements also. Regarding AI review: ## Testing coverage The testing coverage is a legit concern, I assume most test coverage already lives in existing operator/sql level tests, comprehensive UTs on newly refactored utilities are redundant. Now we got configuration `config.enable_migration_hash_join` default to true, so it means the existing tests are all routed to the new impl, the original implementation are left uncovered by test. I see two ways to solve the issue 1. Fuzz testing suite, basically temporarily repeat `config.enable_migration_hash_join` both to true and false, for crate/slt/integration/join fuzz tests, it need certain plumbing in testing infra and extra CI running time 2. Do feature/fix freeze on the original implementation (i.e. for open PRs on fixing/improve HJ, we route them to the rewritten implementation), and try to complete the refactor sooner. I could open a similar PR for existence join shortly, and finish the rewrite atomically. Config is kept for one extra release in case of regressions. We did option 2 for rewriting aggregate: - https://github.com/apache/datafusion/issues/22710 ## Others I'll let AI try to reproduce the potential vulnerabilities with behavior tests, and fix them if there are violations. -- 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]
