Repository navigation
feat(sql): PIVOT and UNPIVOT - #219
Merged
Merged
Conversation
PIVOT and UNPIVOT parsed but lowering rejected them as unsupported. They
now lower to two unresolved IR nodes, PlanNode::Pivot and
PlanNode::Unpivot, built over the relation they follow (per relation and at
the FROM-clause level, as the AST places them); an alias becomes a
SubqueryAlias. A FROM clause carrying both is
NOT_ALLOWED_IN_FROM.UNPIVOT_WITH_PIVOT. The encoding version is bumped to 4
for the new nodes, and the inventory, generator, printers, walks and
mutation sites cover them. Nothing changes in the executor, the physical
planner or the plan codec.
The analyzer resolves them as Spark's ResolvePivot and ResolveUnpivot do:
* PIVOT becomes an Aggregate grouping every input column that neither the
pivot column nor the aggregates reference, with one aggregate per
(value, aggregate) pair of the form agg(if(pivot_col <=> value, expr,
NULL)) (Spark's non-PivotFirst path, which gives the same results),
bound with the existing aggregate machinery. Output names follow Spark:
the value for one aggregate, value_aggalias or value_<aggregate sql>
otherwise; count(*) counts matching rows; first/last ignore the nulls the
if introduces. Pivot values must be foldable and castable to the pivot
column type (NON_LITERAL_PIVOT_VALUES, PIVOT_VALUE_DATA_TYPE_MISMATCH);
an aggregate expression that is not an aggregate is rejected as Spark
rejects it.
* UNPIVOT becomes the existing Expand (one projection per column set: the
kept columns, the name literal, the values) and, unless INCLUDE NULLS, a
filter on coalesce(values) IS NOT NULL. Value columns take the wider
common type of their sources, through the coercion rules
(UNPIVOT_REQUIRES_VALUE_COLUMNS, UNPIVOT_VALUE_DATA_TYPE_MISMATCH,
UNPIVOT_VALUE_SIZE_MISMATCH, UNPIVOT_REQUIRES_ATTRIBUTES).
Multi-column pivots need struct values, which the engine cannot value yet:
once the values are validated they fail with KRISHIV_UNSUPPORTED_FEATURE
("multi-column PIVOT"), never with a wrong result.
Tests: unit tests for lowering and for each analyzer rule and error, and an
end-to-end file replaying the Spark 4.2.0 pivot.sql and unpivot.sql goldens
over registered tables (schema with generated names, rows, error class and
message parameters). Statements that read array, map or struct columns
cannot run in the engine and are skipped by the replay with their reason;
the multi-column pivot statements are asserted to fail as unsupported.
Not verified: Spark Connect's DataFrame pivot is untouched and still
rejected. Diff coverage was not measured locally.
Signed-off-by: techysneha14 <343708768+shehatathe@users.noreply.github.com>
…ck, chained PIVOT/UNPIVOT rejection count(*) and first/last rewrites gain session row tests over courseSales so a wrong rewrite fails instead of only changing plan shape; the pivot_counts_star plan walk now matches the bound when form. Multi-column values with the right arity but an uncastable part fail with PIVOT_VALUE_DATA_TYPE_MISMATCH before the unsupported path. A relation carrying both PIVOT and UNPIVOT lowers to NOT_ALLOWED_IN_FROM.UNPIVOT_WITH_PIVOT, as Spark reports for the pipe-operator chain in the goldens; the lowering snapshot records the rejection. Softened the PivotFirst equivalence comments. Signed-off-by: techysneha14 <343708768+shehatathe@users.noreply.github.com>
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
| Layer / File(s) | Summary |
|---|---|
Plan node contracts and integration crates/krishiv-plan/src/plan.rs, lib.rs, walk.rs, display.rs, encode.rs, tests/* |
Adds unresolved PIVOT and UNPIVOT nodes and public data types. Adds child and expression traversal, unresolved-state handling, display and encoding support, and corresponding test coverage. |
SQL lowering crates/krishiv-sql/src/lower.rs, crates/krishiv-sql/tests/lower.rs |
Lowers relation-level and FROM-clause PIVOT and UNPIVOT into plan nodes. Tests cover aliases, joins, and the error when both extensions appear together. |
Analyzer resolution and validation crates/krishiv-plan/src/analyzer/*, spark_fmt.rs, tests/pivot.rs, crates/krishiv-session/tests/pivot_unpivot.rs |
Resolves PIVOT into filtered aggregates and UNPIVOT into an Expand. Adds validation for values, aggregates, columns, and types, plus unit and Spark golden tests. |
Priority:
Estimated code review effort: 4 (Complex) | ~50 minutes
Possibly related PRs
- KrishivAI/krishiv-engine#150: Adds the Expand plan node used by UNPIVOT resolution.
- KrishivAI/krishiv-engine#217: Changes the visibility of
Resolver::is_aggregate, which PIVOT validation uses. - KrishivAI/krishiv-engine#143: Adds expression constant folding used to validate PIVOT values.
Merge Risk: 
35ca2
-
This change adds SQL PIVOT and UNPIVOT support through SQL lowering and analyzer resolution, with unit tests and Spark golden-file tests. No actionable merge-blocking risk was found. Multi-column pivots are rejected explicitly as unsupported rather than silently mishandled.
Security architecture risk: 
35ca2
-
The new SQL operations reuse existing authorization checks and execution operations. No introduced authorization bypass was established in the inspected paths. Remaining uncertainty concerns external compatibility and deployment exposure, rather than a demonstrated security defect.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — Attacker-supplied SQL gains new transformation paths over its input relations, not an independently evidenced grant of data access. In the inspected flow, those relations still pass through binding and access-policy decisions before resolution.
Trust Boundaries and Controls
- observed — The existing access check uses the current principal and SELECT privilege, rejects denials and policy errors, and rejects row-filter or column-mask decisions as unsupported. The implicit-column bookkeeping difference therefore does not demonstrate a bypass of currently effective column controls.
Resilience and Maintainability Implications
- observed — Resolution creates fresh per-call identity, memoization, column-use, and folding state. AnalyzedPlan is returned only after complete resolution and final plan validation, so a failure after UNPIVOT expansion does not return a partially analyzed plan or publish its local resolver state.
Hardening Proposals
- proposed — Before relying on SecurableUse.columns for column-level enforcement or complete auditing, explicitly account for implicit PIVOT grouping and UNPIVOT kept columns rather than depending on a later SELECT to record them. This is future control-drift prevention, not an observed authorization vulnerability.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Description check The description gives a detailed summary and test scope, but it omits the required task/spec/ADR links, exact test commands with result lines, and completed checklist statuses. Add the task/spec/ADR references, list each test command with its result, and complete the applicable checklist items. Mark skipped or not-applicable items explicitly. Docstring Coverage Docstring coverage is 35.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 126 functions across 19 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly identifies the main change: adding SQL PIVOT and UNPIVOT support. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
feat/sql-pivot-unpivot
- Resolve merge conflict in branch
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Signed-off-by: techysneha14 <343708768+shehatathe@users.noreply.github.com> # Conflicts: # crates/krishiv-plan/src/analyzer/mod.rs # crates/krishiv-plan/src/display.rs # crates/krishiv-plan/src/encode.rs # crates/krishiv-plan/src/plan.rs # crates/krishiv-plan/src/tests/inventory.rs # crates/krishiv-plan/src/tests/props.rs # crates/krishiv-plan/src/walk.rs # crates/krishiv-sql/src/lower.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance1 warning
Comparing 
Footnotes
|
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
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.
PIVOT and UNPIVOT parsed but lowering rejected them as unsupported. They
now lower to two unresolved IR nodes, PlanNode::Pivot and
PlanNode::Unpivot, built over the relation they follow (per relation and at
the FROM-clause level, as the AST places them); an alias becomes a
SubqueryAlias. A FROM clause carrying both is
NOT_ALLOWED_IN_FROM.UNPIVOT_WITH_PIVOT. The encoding version is bumped to 4
for the new nodes, and the inventory, generator, printers, walks and
mutation sites cover them. Nothing changes in the executor, the physical
planner or the plan codec.
The analyzer resolves them as Spark's ResolvePivot and ResolveUnpivot do:
pivot column nor the aggregates reference, with one aggregate per
(value, aggregate) pair of the form agg(if(pivot_col <=> value, expr,
NULL)) (Spark's non-PivotFirst path, which gives the same results),
bound with the existing aggregate machinery. Output names follow Spark:
the value for one aggregate, value_aggalias or value_
otherwise; count(*) counts matching rows; first/last ignore the nulls the
if introduces. Pivot values must be foldable and castable to the pivot
column type (NON_LITERAL_PIVOT_VALUES, PIVOT_VALUE_DATA_TYPE_MISMATCH);
an aggregate expression that is not an aggregate is rejected as Spark
rejects it.
kept columns, the name literal, the values) and, unless INCLUDE NULLS, a
filter on coalesce(values) IS NOT NULL. Value columns take the wider
common type of their sources, through the coercion rules
(UNPIVOT_REQUIRES_VALUE_COLUMNS, UNPIVOT_VALUE_DATA_TYPE_MISMATCH,
UNPIVOT_VALUE_SIZE_MISMATCH, UNPIVOT_REQUIRES_ATTRIBUTES).
Multi-column pivots need struct values, which the engine cannot value yet:
once the values are validated they fail with KRISHIV_UNSUPPORTED_FEATURE
("multi-column PIVOT"), never with a wrong result.
Tests: unit tests for lowering and for each analyzer rule and error, and an
end-to-end file replaying the Spark 4.2.0 pivot.sql and unpivot.sql goldens
over registered tables (schema with generated names, rows, error class and
message parameters). Statements that read array, map or struct columns
cannot run in the engine and are skipped by the replay with their reason;
the multi-column pivot statements are asserted to fail as unsupported.
Not verified: Spark Connect's DataFrame pivot is untouched and still
rejected. Diff coverage was not measured locally.