Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (14)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughSort fields can now use either a sort direction or a custom comparison function. Core protobuf conversion supports both forms. RANGE-frame validation rejects custom comparison ordering, and Spark and Calcite converters report custom comparison sort fields as unsupported. ChangesCustom comparison sort fields
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SortField as Expression.SortField
participant Encoder as ExpressionProtoConverter
participant Proto as Protobuf SortField
participant Decoder as ProtoExpressionConverter
participant Lookup as lookup
SortField->>Encoder: Convert with toProto
Encoder->>Proto: Write expression and comparison-function reference
Proto->>Decoder: Convert with fromSortField
Decoder->>Lookup: Resolve referenced scalar function
Lookup-->>Decoder: Return scalar function variant
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The custom-comparison sort-field changes are mergeable after normal checks; no actionable issue was established. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Custom comparisons now round-trip through the core model, while the inspected Spark and Calcite adapters reject comparisons they cannot represent. The public API change still warrants review, and downstream integration coverage is incomplete. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a8aa30d to
cd90db5
Compare
nielspardon
left a comment
nielspardon
left a comment
There was a problem hiding this comment.
Looks good overall. Two small things: Spark aggregate measures still accept a comparison-function sort without complaint, and fromSortField should reject an unset sort_kind itself instead of going through the direction branch.
| private def toSortOrder(sortField: SExpression.SortField): SortOrder = { | ||
| val expression = sortField.expr().accept(expressionConverter, EmptyVisitationContext.INSTANCE) | ||
| val (direction, nullOrdering) = sortField.direction() match { | ||
| if (!sortField.direction().isPresent) { |
There was a problem hiding this comment.
Add this same guard to fromMeasure, just before val filter (line 113). fromMeasure never reads function.sort(), so a measure ordered by a custom comparator now converts as an unordered aggregate. Before this PR the plan failed to read, so this turns a loud failure into a silent one, and it doesn't match the description's "Spark rejects a comparison-function sort field outright". Dropping direction sorts in general is #1094.
if (function.sort().asScala.exists(!_.direction().isPresent)) {
throw new UnsupportedOperationException(
"A sort field using a custom comparison function is not supported")
}| if (s.getSortKindCase() == SortField.SortKindCase.COMPARISON_FUNCTION_REFERENCE) { | ||
| return Expression.SortField.builder() | ||
| .expr(from(s.getExpr())) | ||
| .comparisonFunction( | ||
| lookup.getScalarFunction(s.getComparisonFunctionReference(), extensions)) | ||
| .build(); | ||
| } | ||
| return Expression.SortField.builder() | ||
| .direction(Expression.SortDirection.fromProto(s.getDirection())) | ||
| .expr(from(s.getExpr())) | ||
| .direction(Expression.SortDirection.fromProto(s.getDirection())) | ||
| .build(); |
There was a problem hiding this comment.
Switch on the oneof case and reject SORTKIND_NOT_SET explicitly. Right now an unset sort kind still fails with Unknown type: SORT_DIRECTION_UNSPECIFIED, and fixing that is the other half of #1093's second item.
| if (s.getSortKindCase() == SortField.SortKindCase.COMPARISON_FUNCTION_REFERENCE) { | |
| return Expression.SortField.builder() | |
| .expr(from(s.getExpr())) | |
| .comparisonFunction( | |
| lookup.getScalarFunction(s.getComparisonFunctionReference(), extensions)) | |
| .build(); | |
| } | |
| return Expression.SortField.builder() | |
| .direction(Expression.SortDirection.fromProto(s.getDirection())) | |
| .expr(from(s.getExpr())) | |
| .direction(Expression.SortDirection.fromProto(s.getDirection())) | |
| .build(); | |
| Expression expr = from(s.getExpr()); | |
| switch (s.getSortKindCase()) { | |
| case DIRECTION: | |
| return Expression.SortField.builder() | |
| .expr(expr) | |
| .direction(Expression.SortDirection.fromProto(s.getDirection())) | |
| .build(); | |
| case COMPARISON_FUNCTION_REFERENCE: | |
| return Expression.SortField.builder() | |
| .expr(expr) | |
| .comparisonFunction( | |
| lookup.getScalarFunction(s.getComparisonFunctionReference(), extensions)) | |
| .build(); | |
| default: | |
| throw new IllegalArgumentException("SortField has no sort_kind set"); | |
| } |
2 participants
Models
SortField.comparison_function_reference, which the POJO previously had no representation for, causing a plan using it to fail to read.SortField.direction()becomesOptional<SortDirection>, paired with a newcomparisonFunction(), with a@Value.Checkenforcing exactly one is set.Both proto conversion directions are updated, along with the RANGE-window ordering validation added for #1198, which now also rejects a custom comparison function on a RANGE bound's ordering expression — closing the non-type-compatibility half of that issue (the type-compatibility half remains blocked on #1227). isthmus and Spark reject a comparison-function sort field outright: Calcite's and Spark's collation models have no representation for a custom comparator.
Also folds
RelProtoConverter's independentSortFieldproto-conversion logic intoExpressionProtoConverter's, which it had been duplicating.Closes #1330
BREAKING CHANGE:
Expression.SortField.direction()now returnsOptional<SortDirection>instead ofSortDirection.Summary by CodeRabbit
New Features
Bug Fixes