Repository navigation
Align ASOF validation diagnostics with Snowflake - #123
Merged
osipovartem merged 2 commits intoOct 9, 2026
Merged
Conversation
osipovartem
marked this pull request as ready for review
October 9, 2026 10:12
osipovartem
merged commit Oct 9, 2026
488c86d
into
df55-csv-numeric-boolean-copy
33 of 38 checks passed
osipovartem
pushed a commit
that referenced
this pull request
Oct 10, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> See the previous PR for rationale - apache#25495 This PR adds #[doc(hidden)] and comments for public APIs that is used as internal utility. The major categories are - already includes `#doc(hidden)` - Already explained in the comment that this public API is internal-only - Optimizer utilities, with strict preconditions to use it correctly. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> comments only ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
…ache#25363) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes #. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> Hash joins can spend substantial time looking up probe keys that do not exist on the build side. For a single integer key with a bounded range, an exact membership bitmap can reject these keys cheaply before hash table lookup. This helps when many nonmatching keys reach the join, including when dynamic filters cannot be pushed down to the input. For example, in the measured TPC-H SF1 plans, the `RightSemi` join on `part.p_partkey = lineitem.l_partkey` receives approximately 6 million probe rows. With memtable inputs or the default Parquet configuration: - **Q8:** Only **0.73%** of probe rows match the filtered build side, resulting in approximately 5.96 million unsuccessful hash table lookups. - **Q9:** Only **5.32%** of probe rows match the filtered build side, resulting in approximately 5.68 million unsuccessful hash table lookups. These low probe hit rates leave substantial room to avoid unsuccessful lookups, reflected in both the query-latency and operator-level measurements below. The extra membership check is less useful when most keys match. An adaptive policy pauses filtering when the observed pruning ratio is low and periodically samples again as the input changes. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> - Build an optional integer membership bitmap for eligible single-key HashMap joins in both CollectLeft and Partitioned modes. ArrayMap and null-aware joins retain their existing paths. - Generate a lookup mask for each probe batch, skipping hash table lookups for absent keys. Preserve the original probe rows and NULL handling so outer, anti, and mark join semantics remain unchanged. Hash computation is unchanged. - Track pruning independently in each probe stream. Unproductive sampling windows trigger progressively longer pauses; productive samples reset the pause interval. - Account for bitmap and mask memory through the memory pool, falling back to normal lookup when an optional reservation cannot be satisfied. - Add configuration, documentation, creation/pruning metrics, and the `hash_join_probe` microbenchmark. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> - Unit tests cover membership and integer boundaries, range overflow, bitmap boundaries, adaptive sampling/backoff, and sliced validity masks. - HashJoin tests cover fallback paths, memory reservation failures, and independent probe-stream adaptation with a shared build bitmap. - `hash_join_integer_prefilter.slt` covers duplicate matches, unmatched rows, NULL equality, inner/full/semi/anti joins, null-aware IN/NOT IN behavior, range limits, and creation/pruning metrics. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. --> The feature is opt-in and disabled by default. It can be enabled with: ```sql SET datafusion.execution.enable_join_integer_prefilter = true; ``` Additional execution options control the maximum key range (default `262144`), minimum pruning ratio (`0.5`), and sampling window (`12288` input rows). The metrics `integer_prefilter_created_count` and `probe_prefilter_rows_pruned` expose whether the prefilter was created and how many probe rows skipped lookup. Query results are unchanged. ## Benchmarks ### Microbench: `hash_join_probe` | Workload | Main | Off | On | On vs main | |---|---:|---:|---:|---:| | RightSemi, 10% interleaved hits | 5.335 | 5.293 | 3.985 | -25.3% | | RightAnti, 10% interleaved hits | 10.849 | 10.933 | 9.479 | -12.6% | | Inner, 10% interleaved hits | 6.114 | 6.164 | 4.802 | -21.4% | | RightSemi, 10% clustered hits | 4.286 | 4.338 | 3.549 | -17.2% | | RightAnti, 10% clustered hits | 10.172 | 11.633 | 8.969 | -11.8% | | RightSemi, all hits | 13.066 | 12.897 | 13.061 | -0.0% | | RightAnti, all hits | 5.920 | 5.924 | 6.112 | +3.2% | | Inner, all hits | 19.235 | 19.567 | 19.565 | +1.7% | | RightSemi, high-to-low hit rate | 9.096 | 9.405 | 9.203 | +1.2% | | RightAnti, high-to-low hit rate | 8.480 | 8.478 | 8.408 | -0.8% | | RightSemi, single probe batch, all hits | 0.205 | 0.217 | 0.258 | +25.9% | | RightSemi, dense 100K build, all hits | 10.172 | 10.264 | 10.107 | -0.6% | | RightSemi, sparse 100K build, all hits | 15.579 | 15.568 | 15.567 | -0.1% | ### TPC-H with memtable | Query | Main | Off | On | On vs main | |---|---:|---:|---:|---:| | Q8 | 25.332 | 23.381 | 16.080 | -36.5% | | Q9 | 48.990 | 46.381 | 41.653 | -15.0% | | Q17 | 67.344 | 66.039 | 43.156 | -35.9% | ### TPC-H with parquet with the default `datafusion.execution.parquet.pushdown_filters=false` | Query | Main | Off | On | On vs main | | ----- | -----: | -----: | -----: | ---------: | | Q8 | 63.736 | 63.243 | 58.271 | -8.6% | | Q9 | 76.014 | 74.103 | 70.175 | -7.7% | ### Target HashJoin `join_time` The following table measures the `CollectLeft` / `RightSemi` HashMap join on `part.p_partkey = lineitem.l_partkey` described above. It reports **operator `join_time`, summed across the four probe partitions**, separately from end-to-end query latency. | Input | Query | Main | Off | On | On vs main | | ----------------- | ----- | -----: | -----: | -----: | ---------: | | MemTable | Q8 | 29.330 | 29.625 | 12.045 | -58.9% | | MemTable | Q9 | 39.985 | 40.010 | 26.415 | -33.9% | | Parquet (default) | Q8 | 29.685 | 29.820 | 11.190 | -62.3% | | Parquet (default) | Q9 | 35.830 | 35.305 | 20.525 | -42.7% |
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> Part of the work towards apache#15914 ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> `concat_ws` return value is always nullable in DF, which is incorrect. Per upstream Spark implementation, this is only true when separator itself is null. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> Derive the nullability based on the separator nullability. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> Add SLT tests for schema. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. --> I believe this doesn't count as an API change but would like guidance from a committer.
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes apache#26168. ## Rationale for this change see issue, the cost for hashing dictionary arrays comes down to a writing corresponding values to their key indices, we can speed this up with branchless code. <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> ## What changes are included in this PR? Rewrites the scatter as four const-generic specializations: - 8-way unrolled gather for the no-nulls case - single-check loop for nullable values - 64-key bitmask-driven path for nullable keys (branchless gather+csel for valid values, `trailing_zeros` bit-walk otherwise) - scalar tail for non-multiple-of-64 lengths <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> ## What is the testing strategy for this PR? existing test cover this path <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ## Are there any user-facing changes? no <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> no related issue ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> `OptimizerRule::supports_rewrite` has been deprecated since DataFusion 47.0.0 and is no longer called by the optimizer. Its deprecation exceeds both the six-major-version and six-month retention periods in the [API health policy](https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines). ## What changes are included in this PR? remove `OptimizerRule::supports_rewrite` <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
apache#24105) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> N/A. Continuation of apache#23609. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Improve perf of TopK. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Eliminate overhead of casting with `dyn Any` for every String in `topk::StringHeap`. Rearrange `HashTableItem<ID>` `Option`-ness to avoid unnecessary unwraps on lookups in `topk::TopKHashTable`. Rewrite inserts to take a borrowable type, delaying clones and allowing reuse of storage slots (mostly Strings). ## Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> Should be covered by existing tests. Making `ID` an `Option<ID>` inside `HashTableItem` should be fine, since the internal `HashTable` keeps track of valid items (although this could be hard to verify properly). It runs with the assumption that "if everything needed an unwrap, nothing needs an unwrap". ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> No. <!-- If there are any breaking changes to public APIs, please add the `api change` label. --> --------- Co-authored-by: kosiew <kosiew@gmail.com>
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - None; documentation fix. Follows up apache#22314. ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. Please explain the problem you are trying to solve in terms of the user-visible behavior, rather than the implementation. For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the implementation. "COUNT(DISTINCT) returns wrong results when the column contains nulls" is the user-visible problem. --> The operators page documents `#` as bitwise xor, "interchangeable with `^`". On `main`, `SELECT 5 # 3;` fails with the default dialect (`ParserError("No infix parser for token Sharp ...")`); `#` is only xor in the PostgreSQL and Redshift dialects. The page describes the default dialect, where `^` is xor. ## What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR. --> The xor section and its index entry now document `^` instead of `#`, with the current column name (`BIT_XOR`) in the example output. Per review, the page sticks to the default dialect rather than listing per-dialect exceptions. ## What is the testing strategy for this PR? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> Ran the new example against `datafusion-cli` built from `main`; `./ci/scripts/doc_prettier_check.sh` passes. Docs only, no test. ## Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. --> Documentation only. Generated with Claude Code (claude-opus-5-5). It came from running the documented SQL examples against `main`, not from an issue.
osipovartem
pushed a commit
that referenced
this pull request
Oct 15, 2026
apache#25710) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123. --> - Closes apache#25660. ## Rationale for this change See apache#25660. This change only documents/enforces this requirement for built-in aggregate UDF functions. Anyone who implements their own aggregate UDFs do not need to adhere to this requirement since vanilla datafusion doesn't require it. This does add a public test that user's can use to make sure this requirement is satisfied if they desire. ## What changes are included in this PR? This change adds: - Internal documentation explaining this requirement and how to check for it using the provided test function. - `datafusion_functions_aggregate::testing::check_state_compatibility()`: A generic test that identifies cases where the `Accumulator` and `GroupsAccumulator` of an `AggregateUDF` differ. This is exposed as a public function that can be used to test this requirement on `AggregateUDFImpl`s implemented by downstream consumers. The test is run on all built-in default aggregation functions. - Fixes for `approx_distinct()` and `avg()`, which both violated this requirement - This takes the changes from apache#25659 to fix `approx_distinct()` - The `Accumulator` for `avg()` would return `0` for the count if it hasn't seen any non-null values. The `GroupsAccumulator` would return `null` instead. This change updates the `Accumulator`s to also return `null` for the count in this case. ## What is the testing strategy for this PR? This adds a test to enforce this new requirement in a generic way. <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code Briefly describe how this PR is tested, and point to the specific tests you added. For example: 'This new feature is covered by the `sqllogictest` cases added in `foo.slt`'. If this PR does not add tests, explain why. For example, if the change is already covered by existing tests, please mention it. You should also check the `codecov` bot reply on this PR to confirm the changed code is exercised. --> ## Are there any user-facing changes? This changes the intermediate state of the `avg()` accumulator to match the `avg()` groups accumulator, which some users may be affected by. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please add the `api change` label. -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
cargo test -p datafusion-expr asof_join_on_extracts_and_validates_conditions --lib: passed.cargo test -p datafusion-sqllogictest --test sqllogictests -- asof_join: passed.cargo fmt --all -- --check: passed.Companion Rustice PR pins the runtime commit and updates its expectations and live evidence.