fix: Fix append regression in 0.13.5#5495
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a regression in the append functionality introduced in version 0.13.5. The fix addresses issue #5494 by improving the positional mapping logic for UNION operations in SQL generation.
- Adds bounds checking to prevent invalid array access when applying positional mappings
- Ensures positional mappings are only stored when complete (all columns matched)
- Adds defensive programming with logging for better debugging
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| prqlc/prqlc/src/sql/pq/positional_mapping.rs | Core fix adding bounds validation and completeness checks for positional mappings |
| prqlc/prqlc/tests/integration/queries/append_cte_complex.prql | New test case demonstrating complex append scenarios with CTEs |
| Multiple snapshot files | Updated test snapshots reflecting the regression fix and new test additions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
thanks @priithaamer . sorry for the regression. let's merge? or any other feedback? |
|
I did the same re reducing the integration tests for this — https://github.com/max-sixty/prql/tree/fix-5494-append-regression (or I can push to this MR with perms) |
|
@priithaamer Could you take a look at this? |
Replace the comprehensive integration test with a focused inline test that verifies the append with CTEs regression fix. The test ensures positional mapping doesn't cause out-of-bounds errors when using append with let statements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
@eitsupi sorry for the delay. I've merged improvements from https://github.com/max-sixty/prql/tree/fix-5494-append-regression |
Attempt to fix #5494