feat: Support swap for RightMark Join - #17651
Merged
Merged
Conversation
github-actions
Bot
added
logical-expr
Contributor
Author
|
@comphead Decided to circle back to this after forgetting that this was reverted 😆 Are you able to run extended tests? |
Contributor
|
I think we got something broken, you can still run extended tests on your fork, against your branch |
jonathanc-n
marked this pull request as draft
September 18, 2025 21:43
jonathanc-n
marked this pull request as ready for review
September 19, 2025 01:37
Contributor
Author
|
@comphead It is passing on my end. I added the slt test that is the query that caused the error from the previous pull request. |
Contributor
Author
|
cc @comphead I believe this should be good for another look! |
comphead
reviewed
Sep 29, 2025
| .partition_statistics(None) | ||
| .unwrap() | ||
| .total_byte_size, | ||
| Precision::Inexact(2097152) |
Contributor
There was a problem hiding this comment.
can this number be a reason of flaky test in the future?
Contributor
Author
There was a problem hiding this comment.
I don't think so, i have copied this from the other previous join selection swapping testing and thos ehave been working fine.
comphead
reviewed
Sep 29, 2025
comphead
approved these changes
Sep 29, 2025
comphead
left a comment
comphead
left a comment
Contributor
There was a problem hiding this comment.
Thanks @jonathanc-n I think the PR is good to go
comphead
reviewed
Sep 30, 2025
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
Vedin
added a commit
to Embucket/datafusion
that referenced
this pull request
Jul 2, 2026
… column, stale EXPLAIN plans - to_date: Float64 sat in the direct-cast arm, but arrow has no Float64 -> Date32 cast; route it through the Int64 two-step like Float16/Float32/Decimal (dates.slt:216 now passes, truncation semantics identical to the other float widths). - PIVOT: the value column was rendered with Expr::to_string, which keeps quote characters, so a quoted pivot column (FOR "2023_Q1" IN ...) never matched its schema field and was not consumed by the pivot. Normalize the identifier like the rest of the planner (pivot.slt nested-PIVOT case now passes). - subquery.slt: regenerate the two EXPLAIN expectations — the plans changed upstream (CoalesceBatchesExec deprecated in apache#19622, LeftMark -> RightMark swap added in apache#17651) and were never regenerated during the df53 sync; physical-plan shape only, logical plans unchanged. encrypted_parquet.slt still fails locally only (expects a no-keys read to error; environment-dependent, fails identically on the clean base, passes in CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Vedin
added a commit
to Embucket/datafusion
that referenced
this pull request
Jul 2, 2026
…cs) (#43) * Decimal division and AVG round half away from zero (Snowflake semantics) Snowflake rounds the last digit of decimal division and AVG results half away from zero (verified live: 5/3 = 1.666667, -5/3 = -1.666667, 5/2000000 = 0.000003 — not half-even, not truncation). Arrow's div kernel and DecimalAverager::avg both truncate toward zero, which produced the persistent <=1 ULP parity gap on TPC-H Q1/Q8 and ClickBench Q28. - DecimalAverager::avg: correct the truncated quotient with the remainder. - BinaryExpr Divide on decimals: dispatch to a fork-local kernel that replicates arrow-arith's decimal Op::Div exactly (result precision/scale derivation, overflow and divide-by-zero errors) but rounds half away from zero. Forking arrow itself was rejected as far heavier. - decimal.slt expectations regenerated; pre-existing failures in subquery/dates/encrypted_parquet/pivot slt files are unchanged (verified identical on the clean base). - Bump the workspace sqlparser pin to the Snowflake string-escape fix rev (074bd21) so downstream consumers resolve a single sqlparser instance. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Point sqlparser at the merged sync-branch head (01e9e5e) The Snowflake string-escape fix was squash-merged into embucket-sync-df53.0.0-parser0.61.0 as 01e9e5e (content-identical to the previously pinned 074bd21 feature-branch rev). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix pre-existing sqllogictest failures: to_date(double), quoted PIVOT column, stale EXPLAIN plans - to_date: Float64 sat in the direct-cast arm, but arrow has no Float64 -> Date32 cast; route it through the Int64 two-step like Float16/Float32/Decimal (dates.slt:216 now passes, truncation semantics identical to the other float widths). - PIVOT: the value column was rendered with Expr::to_string, which keeps quote characters, so a quoted pivot column (FOR "2023_Q1" IN ...) never matched its schema field and was not consumed by the pivot. Normalize the identifier like the rest of the planner (pivot.slt nested-PIVOT case now passes). - subquery.slt: regenerate the two EXPLAIN expectations — the plans changed upstream (CoalesceBatchesExec deprecated in apache#19622, LeftMark -> RightMark swap added in apache#17651) and were never regenerated during the df53 sync; physical-plan shape only, logical plans unchanged. encrypted_parquet.slt still fails locally only (expects a no-keys read to error; environment-dependent, fails identically on the clean base, passes in CI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
3 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.
Which issue does this PR close?
Part of fix for #16488
Changes
During decorrelation of an exists subquery into a LeftMark join, the inner side still carries the literal from 'select 1'. The schema looks like this:
right = Projection: [Int64(1), x.b]Projection Pushdown later drops the unused constant and rewrites the right child to just [x.b]. That “drop the first column” step shifts right-side column indices (from b@1 to b@0). During projection mapping it still assumes the old index (often doing “offset by left_width” and not accounting for the mark column), so when it re-indexes into the child it points at the wrong slot (which is what hit the error)
to fix this, I removed the projections when optimizer builds the join
Tests
The test is added to slt.