slack-15.0: backport vitessio/vitess#16154
#420
Conversation
|
Thanks for the contribution! Before we can merge this, we need @systay to sign the Salesforce Inc. Contributor License Agreement. |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
|
Thanks for the contribution! Before we can merge this, we need @systay @GuptaManan100 to sign the Salesforce Inc. Contributor License Agreement. |
| @@ -878,6 +878,18 @@ func (cached *Count) CachedSize(alloc bool) int64 { | |||
| } | |||
| return size | |||
| } | |||
| func (cached *CountStar) CachedSize(alloc bool) int64 { | |||
There was a problem hiding this comment.
@GuptaManan100 any idea why *CountStar didn't have this method already? 🤔
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
| // | ||
| // The solution we employed was to add a dummy field `_ bool` to the otherwise empty struct `CountStar`. | ||
| // This ensures that each instance of `CountStar` is treated as a separate object, | ||
| // even in the context of out semantic state which uses these objects as map keys. | ||
| } |
There was a problem hiding this comment.
Noting that I didn't include the OverClause field from the upstream change - v15 doesn't have the OverClause support
There was a problem hiding this comment.
Context on OverClause: vitessio#13444
| @@ -54,6 +59,10 @@ func NewMySQLCompare(t *testing.T, vtParams, mysqlParams mysql.ConnParams) (MySQ | |||
| }, nil | |||
| } | |||
|
|
|||
| func (mcmp *MySQLCompare) AsT() *testing.T { | |||
There was a problem hiding this comment.
I don't think anything calls this, but in the upstream change something does. I'll just leave it here
There was a problem hiding this comment.
This is only used by https://github.com/vitessio/vitess-tester, AFAIK
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
|
Noting that I did not backport the changes to |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
| @@ -2656,8 +2656,12 @@ func (node *Count) Format(buf *TrackedBuffer) { | |||
| } | |||
|
|
|||
| func (node *CountStar) Format(buf *TrackedBuffer) { | |||
| buf.astPrintf(node, "%s(", node.AggrName()) | |||
| buf.WriteString("*)") | |||
| if node.Name != "" { | |||
There was a problem hiding this comment.
@GuptaManan100 I'd like a 👍 on this fix because it wasn't in the original patch. Without the fallback to node.AggrName() many tests were returning (*) instead of COUNT(*)
There was a problem hiding this comment.
I'm no mr Gupta, but you can have my 👍
I was surprised we didn't do this earlier
There was a problem hiding this comment.
Yep, this would work too I think
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
|
Added a few more tests to be extra safe. cc @vmogilev / @tanjinx / @GuptaManan100 for review/thoughts 🙇 |
| @@ -2707,6 +2707,34 @@ type ( | |||
| } | |||
|
|
|||
| CountStar struct { | |||
| Name string | |||
There was a problem hiding this comment.
@GuptaManan100 would this reduce the cost of this change? Enough to matter?
type CountStarName int
var (
CountStarLower CountStarName = iota
CountStarCapital
CountStarUpper
CountStarNameStr map[CountStarName]string{
CountStarLower: "count(*)",
CountStarCapital: "Count(*)",
CountStarUpper: "COUNT(*)",
}
)
func (csn CountStarName) String() string {
return CountStarNameStr[csn]
}
or a more generic:
var (
AggregatorLower AggregatorName = iota
AggregatorCapital
AggregatorUpper
)
func (an AggregatorName) Name(name string) string {
switch an {
case AggregatorLower:
return strings.ToLower(name)
case AggregatorCapital:
return strings.Title(name)
case AggregatorUpper:
return strings.ToUpper(name)
}
}
(count, Count and COUNT only)
Name string would become Name CountStarName in CountStar struct
There was a problem hiding this comment.
It could matter. It would definitely reduce the size of the struct, but how meaningful that overall change is going to be to runtime, we'll have to test. I don't think it would be very significant either way, because we do need to print the string eventually during formatting.
There was a problem hiding this comment.
@GuptaManan100 makes sense. I think we aren't too worried about something this finite, so I suppose I'm just thinking aloud how a potential fix to main could be done without adding overhead
|
This PR is being marked as stale because it has been open for 30 days with no activity. To rectify, you may do any of the following:
If no action is taken within 7 days, this PR will be closed. |
|
This PR was closed because it has been stale for 7 days with no activity. |
Description
This PR backports vitessio#16154, also vitessio#15581 to make the change easier to integrate. Also small tweaks were made for cases where
node.Nameis""This change hopes to address problems with case-sensitivity with aggregation functions in
SELECTsRelated Issue(s)
Checklist
Deployment Notes