[SPARK-42746][SQL][FOLLOWUP] Improve the code for SupportsOrderingWithinGroup and Mode #49907
Conversation
There was a problem hiding this comment.
cc @mikhailnik-db , @cloud-fan because this is a follow-up of
| @@ -390,10 +390,6 @@ case class PercentileCont(left: Expression, right: Expression, reverse: Boolean | |||
| override protected def withNewChildrenInternal( | |||
| newLeft: Expression, newRight: Expression): PercentileCont = | |||
| this.copy(left = newLeft, right = newRight) | |||
|
|
|||
| override def orderingFilled: Boolean = left != UnresolvedWithinGroup | |||
There was a problem hiding this comment.
This is always false now?
There was a problem hiding this comment.
Why do we make such a change?
There was a problem hiding this comment.
Before the PR #48748, the default value is false.
That is a change not necessary!
There was a problem hiding this comment.
why it's not necessary? orderingFilled should be true if UnresolvedWithinGroup is resolved and replaced?
There was a problem hiding this comment.
Because the left should be always UnresolvedWithinGroup with PercentileContBuilder.
There was a problem hiding this comment.
I think the previous version is better as we don't need this hidden assumption. I also don't think this PR improves the code, as requiring the implementation to implement these method can force them to think about the semantic.
| /** Indicator that ordering was set. */ | ||
| def orderingFilled: Boolean | ||
| def orderingFilled: Boolean = false | ||
|
|
||
| /** | ||
| * Tells Analyzer that WITHIN GROUP (ORDER BY ...) is mandatory for function. | ||
| * | ||
| * @see [[QueryCompilationErrors.functionMissingWithinGroupError]] | ||
| */ | ||
| def isOrderingMandatory: Boolean | ||
| def isOrderingMandatory: Boolean = true | ||
|
|
||
| /** | ||
| * Tells Analyzer that DISTINCT is supported. | ||
| * The DISTINCT can conflict with order so some functions can ban it. | ||
| * | ||
| * @see [[QueryCompilationErrors.functionMissingWithinGroupError]] | ||
| * @see [[QueryCompilationErrors.distinctWithOrderingFunctionUnsupportedError]] | ||
| */ | ||
| def isDistinctSupported: Boolean | ||
| def isDistinctSupported: Boolean = false |
There was a problem hiding this comment.
I don't like the idea of default flags in this trait. I think every developer should make a deliberate decision about what behavior he wants for a new function. With default values, it's easy to forget some corner cases.
Personally, as a new contributor, I saw abstract/non-implemented methods and flags as hints, which is not to forget. But about default values, you can get to know them only by reading all the code or during testing.
@cloud-fan, what do you think?
What changes were proposed in this pull request?
This PR propose to improve the code for
SupportsOrderingWithinGroupand ModeWhy are the changes needed?
First, we can simplify the code by given the default value to
SupportsOrderingWithinGroup.Second, Correct the comments style.
Does this PR introduce any user-facing change?
'No'.
New feature.
How was this patch tested?
GA.
Was this patch authored or co-authored using generative AI tooling?
'No'.