viirya commented on PR #26214: URL: https://github.com/apache/datafusion/pull/26214#issuecomment-6547679621
Thanks @comphead. The remaining round-one items are in 043950b04 and 0eb4fda5b: - `null_aware_scope_map_built_only_for_null_probe_values` now also runs the low-memory case, and the separate fallback test is gone. - The third `roundtrip_join_null_aware` case is dropped. - The `(b, a)`, `(a, k, b)`, scalar `a NOT IN (.. WHERE y = b)`, `OR value = 'q'` and empty-subquery queries are dropped from `null_aware_multi_column.slt`. - An EXPLAIN in `null_aware_anti_join.slt` pins `b = y` as a scope key for the `mc_scope` query. - The Test 19 header describes the scope-key layout. - `hot_scope_among_rare_ones` is inlined, and the PR-history sentence is gone. I had missed the design points in your first review: - Deciding the layout at execution time: agreed, that is where most of the gain is, since most sources declare every column nullable. The build side's logical null count is known once it is collected, but the probe side streams, so a later probe batch can still bring a NULL in an element that has none on the build side. Handling that means choosing the scope keys per probe row, which is a larger change than this PR, so I'll open a follow-up issue for it. - Splitting out the volatile-key fix and the chunked `cross_product`: I'd rather keep them here. Both came out of the review of this PR and have been reviewed here, so a separate PR would mean reviewing them again. - A declined scope map is no longer cached (043950b04). Only a built map is, so the next NULL-valued probe batch tries again. That costs one `try_grow`, because `new_join_hashmap` reserves before it allocates. `declined_scope_map_is_built_once_memory_is_free` covers it. Your round-one inline comments were re-posted, so my replies to them now sit outside their threads: [cast](https://github.com/apache/datafusion/pull/26214#discussion_r4431021621), [stable sort](https://github.com/apache/datafusion/pull/26214#discussion_r4431021706), [`update_hash`](https://github.com/apache/datafusion/pull/26214#discussion_r4431021755), [`on_left`](https://github.com/apache/datafusion/pull/26214#discussion_r4431021804), [groups](https://github.com/apache/datafusion/pull/26214#discussion_r4431021850), [tests](https://github.com/apache/datafusion/pull/26214#discussion_r4431021924), [docs](https://github.com/apache/datafusion/pull/26214#discussion_r4431021989). -- 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]
