`slack-15.0`: backport vitessio/vitess#16154 #420

timvaillancourt · 2024-06-19T14:05:16Z

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.Name is ""

This change hopes to address problems with case-sensitivity with aggregation functions in SELECTs

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

salesforce-cla · 2024-06-19T14:05:20Z

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 <[email protected]>

Signed-off-by: Manan Gupta <[email protected]>

salesforce-cla · 2024-06-19T15:25:46Z

Thanks for the contribution! Before we can merge this, we need @systay @GuptaManan100 to sign the Salesforce Inc. Contributor License Agreement.

timvaillancourt · 2024-06-19T15:27:35Z

go/vt/sqlparser/cached_size.go

@@ -878,6 +878,18 @@ func (cached *Count) CachedSize(alloc bool) int64 {
 	}
 	return size
 }
+func (cached *CountStar) CachedSize(alloc bool) int64 {


@GuptaManan100 any idea why *CountStar didn't have this method already? 🤔

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt · 2024-06-19T15:53:10Z

go/vt/sqlparser/ast.go

+		//
+		// 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.
 	}


Noting that I didn't include the OverClause field from the upstream change - v15 doesn't have the OverClause support

Context on OverClause: vitessio#13444

timvaillancourt · 2024-06-19T16:04:19Z

go/test/endtoend/utils/cmp.go

@@ -54,6 +59,10 @@ func NewMySQLCompare(t *testing.T, vtParams, mysqlParams mysql.ConnParams) (MySQ
 	}, nil
 }

+func (mcmp *MySQLCompare) AsT() *testing.T {


I don't think anything calls this, but in the upstream change something does. I'll just leave it here

This is only used by https://github.com/vitessio/vitess-tester, AFAIK

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt · 2024-06-19T19:48:07Z

Noting that I did not backport the changes to onecase.json because this caused CI trouble and the parser change is well-tested by existing unit tests such as TestValidUnionCases and TestValidSelectCases

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt · 2024-06-19T20:29:07Z

go/vt/sqlparser/ast_format.go

@@ -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 != "" {


@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(*)

I'm no mr Gupta, but you can have my 👍
I was surprised we didn't do this earlier

Yep, this would work too I think

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt · 2024-06-19T23:40:41Z

Added a few more tests to be extra safe. cc @vmogilev / @tanjinx / @GuptaManan100 for review/thoughts 🙇

timvaillancourt · 2024-06-20T00:39:01Z

go/vt/sqlparser/ast.go

@@ -2707,6 +2707,34 @@ type (
 	}

 	CountStar struct {
+		Name string


@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

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.

@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

github-actions · 2024-07-22T01:31:23Z

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:

Push additional commits to the associated branch.
Remove the stale label.
Add a comment indicating why it is not stale.

If no action is taken within 7 days, this PR will be closed.

github-actions · 2024-07-30T01:30:54Z

This PR was closed because it has been stale for 7 days with no activity.

chore: use interface instead of struct for tests (vitessio#15581)

529c16b

salesforce-cla bot added the cla:missing label Jun 19, 2024

github-actions bot added this to the v15.0.5 milestone Jun 19, 2024

timvaillancourt and others added 5 commits June 19, 2024 16:08

Fix bad conflict fix

4ac6278

Signed-off-by: Tim Vaillancourt <[email protected]>

Fix bad conflict fix again

3ee6f07

Signed-off-by: Tim Vaillancourt <[email protected]>

feat: add benchmark for count(*)

31bee01

Signed-off-by: Manan Gupta <[email protected]>

feat: fix the Count(*) function printing

748d9c2

Signed-off-by: Manan Gupta <[email protected]>

feat: fix count star creations from the planner

6de3492

Signed-off-by: Manan Gupta <[email protected]>

timvaillancourt commented Jun 19, 2024

View reviewed changes

timvaillancourt added 2 commits June 19, 2024 17:30

make sizegen

75f552b

Signed-off-by: Tim Vaillancourt <[email protected]>

update cluster.PanicHandler(...)

4ab096b

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt commented Jun 19, 2024

View reviewed changes

make sizegen again

20ee454

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt added upstream-backport An upstream backport v15 labels Jun 19, 2024

timvaillancourt added 6 commits June 19, 2024 20:16

Use node.Name only if set

cb6e9ad

Signed-off-by: Tim Vaillancourt <[email protected]>

make sizegen again

2f10041

Signed-off-by: Tim Vaillancourt <[email protected]>

make sizegen from golang:1.21 container

261f9c5

Signed-off-by: Tim Vaillancourt <[email protected]>

Fix some planner expectations

b978f63

Signed-off-by: Tim Vaillancourt <[email protected]>

Update TestValidUnionCases and TestValidSelectCases

35353c5

Signed-off-by: Tim Vaillancourt <[email protected]>

revert onecase.json

121d127

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt marked this pull request as ready for review June 19, 2024 19:45

timvaillancourt requested a review from a team as a code owner June 19, 2024 19:45

Update TestSelectDBA

ae6c966

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt commented Jun 19, 2024

View reviewed changes

tanjinx previously approved these changes Jun 19, 2024

View reviewed changes

timvaillancourt requested a review from vmogilev June 19, 2024 22:23

vmogilev previously approved these changes Jun 19, 2024

View reviewed changes

Add further testing to CountStar formatting

92cd529

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt dismissed stale reviews from vmogilev and tanjinx via 92cd529 June 19, 2024 23:24

timvaillancourt requested review from tanjinx and vmogilev June 19, 2024 23:25

one more test

7a5457e

Signed-off-by: Tim Vaillancourt <[email protected]>

timvaillancourt commented Jun 20, 2024

View reviewed changes

tanjinx approved these changes Jun 20, 2024

View reviewed changes

vmogilev approved these changes Jun 21, 2024

View reviewed changes

github-actions bot added the Stale label Jul 22, 2024

github-actions bot closed this Jul 30, 2024

timvaillancourt deleted the bp-pr16154-slack-15.0 branch October 9, 2024 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`slack-15.0`: backport vitessio/vitess#16154 #420

`slack-15.0`: backport vitessio/vitess#16154 #420

timvaillancourt commented Jun 19, 2024 •

edited

Loading

salesforce-cla bot commented Jun 19, 2024

salesforce-cla bot commented Jun 19, 2024

timvaillancourt Jun 19, 2024

timvaillancourt Jun 19, 2024 •

edited

Loading

timvaillancourt Jun 19, 2024

timvaillancourt Jun 19, 2024

systay Jun 20, 2024

timvaillancourt commented Jun 19, 2024

timvaillancourt Jun 19, 2024

systay Jun 20, 2024

GuptaManan100 Jun 20, 2024

timvaillancourt commented Jun 19, 2024

timvaillancourt Jun 20, 2024 •

edited

Loading

GuptaManan100 Jun 20, 2024

timvaillancourt Jun 20, 2024

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 30, 2024

slack-15.0: backport vitessio/vitess#16154 #420

slack-15.0: backport vitessio/vitess#16154 #420

Conversation

timvaillancourt commented Jun 19, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

salesforce-cla bot commented Jun 19, 2024

salesforce-cla bot commented Jun 19, 2024

Choose a reason for hiding this comment

timvaillancourt Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvaillancourt commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvaillancourt commented Jun 19, 2024

timvaillancourt Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 22, 2024

github-actions bot commented Jul 30, 2024

`slack-15.0`: backport vitessio/vitess#16154 #420

`slack-15.0`: backport vitessio/vitess#16154 #420

timvaillancourt commented Jun 19, 2024 •

edited

Loading

timvaillancourt Jun 19, 2024 •

edited

Loading

timvaillancourt Jun 20, 2024 •

edited

Loading