Remove the unused StrictComparison::to_compare_operator - #9405
Draft
connortsui20 wants to merge 8 commits into
Draft
Remove the unused StrictComparison::to_compare_operator#9405connortsui20 wants to merge 8 commits into
StrictComparison::to_compare_operator#9405connortsui20 wants to merge 8 commits into
Conversation
`Between` is not strict. Execution falls back to `lower <= arr AND arr <= upper` under Kleene `AND`, so a null bound still yields a definite `false` when the other comparison is false. `validity` conjoined all three children, declaring a row null whenever any bound was null, including rows that execution resolves to `false`. `Binary` returns `None` for `Operator::And` precisely because Kleene `AND` has no derivable validity expression, and `Between` desugars to that same `And`. Return `None` so the expression is evaluated and its mask extracted. Narrowing to the `arr` child would also be unsound, since a row can be legitimately null while `arr` is valid. Add a test that a declared validity agrees with the mask of the executed result, which covers this class of defect beyond `Between`. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`precondition` returned an all-null `ConstantArray` whenever either bound was a constant null. Under Kleene `AND` a null bound only makes a row null when the other comparison is not already false, so this nulled rows the surviving bound had already falsified. Because the branch keys off `as_constant()`, the result was also encoding-dependent: an all-null chunk stored as a `PrimitiveArray` produced `[false, null]` while the same chunk compressed to a `ConstantArray` produced `[null, null]`. Compression encodes all-null chunks as constants, so the same predicate over the same data could disagree from chunk to chunk. This also made `find_between` non-value-preserving. It rewrites conjoined comparisons with literal bounds into `Between`, so a null literal reached `precondition` through the standard optimizer. Under `not(...)` the rewrite changed a query's row count, since `NOT FALSE` is `TRUE` while `NOT UNKNOWN` is `UNKNOWN`. Short-circuit to all null only when both bounds are null, which is the one case where no comparison can falsify a row. With a single null bound, desugar into the two comparisons combined with Kleene `AND`, since the kernels all require non-null constant bounds. Reuse that desugaring for the existing fallback in `between_canonical`. `test_constants` asserted only that no row was `true`, which held under both the old and the correct result, so tighten it to the exact values. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
Removes a contract stated three times. `precondition` documents that its result can be lazy, so the two call sites no longer repeat that in a comment. Replaces the copied null `ConstantArray` construction with a `null_i32s` test helper, and names why that encoding matters. Presents the `validity` test data as an annotated truth table rather than three columns, since the row is the unit the test reasons about. Renames `lower_null` to `lower_is_null` to read as the boolean it is, drops "may" and "should" from the docs, and adds the missing blank lines before returns. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`desugar` never said what it desugars into, and the term is off-register for an array compute crate. `Between` stands for two compares combined with Kleene `AND`, so the name now says that directly. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`precondition` can now return a lazy array, and the two execution paths force it because their callers expect a computed array. Both carry the same debt as the existing fallback TODO in `between_canonical`, so record it where the forcing happens. The reduce adaptor needs no marker, since a reduce rule can return a lazy array. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
Every other non-strict scalar fn in this family explains its `false`, and the trait default is also `false`, so a bare override recorded no intent. The comment states the reason and points at `validity`, matching `Binary`. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
`between_canonical` still described the old `execute_boolean` fallback, so it now links [`as_two_compares`]. Trims the `is_strict` comment to point at `validity` instead of restating its reasoning, matching `Binary`, and settles on "compares" in both. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
Its last caller was the `between_canonical` fallback, which now builds its operators with `to_operator`. Nothing else in the workspace called it. Removing it also drops the `CompareOperator` import from the module. This is a public API removal on a type that eight crates use, so it is split out from the fix that orphaned it. Signed-off-by: "Connor" <connor@spiraldb.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Kc2AuwyEzVQBJLMbfVV36
Merging this PR will degrade performance by 13.85%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | cold_misaligned[(64, 256)] |
4.4 ms | 5.3 ms | -16.96% |
| ❌ | Simulation | compress_fsst[(1000, 64, 8)] |
1 ms | 1.2 ms | -10.63% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/remove-to-compare-operator (41a7eaa) with claude/github-issue-9212-ia0abi (33cbc33)
Footnotes
-
89 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
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
2 participants
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.
Rationale for this change
Its last caller was the
between_canonicalfallback, which #9404 rewrote to build its operators withto_operator. Nothing else in the workspace calls it, and it ispub const fn, so no dead-code lint will report it later.ANDsemantics forBetweennull bounds #9404What changes are included in this PR?
Deletes the method and the
CompareOperatorimport it needed. Split out from #9404 becauseStrictComparisonis public and eight crates use it, so removing a method from it is a separate decision from a semantics fix.What APIs are changed? Are there any user-facing changes?
StrictComparison::to_compare_operatoris gone. No caller exists in the workspace, but it was reachable, so this is a public API removal.to_operatorcovers the same need and returns theOperatorthat callers actually want.Generated by Claude Code