Cleanup whitespace and comments in SQL query text #1246

timvaillancourt · 2023-01-11T22:19:09Z

Description

This PR cleans up SQL query text in several files where whitespace, line separation and the use of comments were inconsistent

The rules I followed were:

All queries include a /* gh-ost */ or /* gh-ost database.table */ comment
Single-tabs (/t) are used for indentation
Queries are defined as the variable query := using backticks
SQL begins on the line following query after a single tab
- Example:
```
query := `
        select /* gh-ost */
        ...`
```
- Exception: simple var-only queries without a FROM, example: select /* gh-ost */ @@global.log_slave_updates
SQL is further indented when these terms are encountered:
- FROM/INTO
- SET/ON DUPLICATE KEY UPDATE
- VALUES
- WHERE
- ( or ) SQL-block brackets, example: a sub-query
- INNER JOIN
- ON
- CASE
No newline at end of query var / close backtick after last word

What this PR does NOT address is the inconsistency in capitalization in SQL. I would like to leave this to a vote from the community in an up-coming GitHub Discussion, and I will follow-up on this separately

In case this PR introduced Go code changes:

contributed code is using same conventions as original code
script/cibuild returns with no formatting errors, build errors or unit test errors.

meiji163 · 2023-12-08T22:10:49Z

looks nice!

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Cleanup whitespace in SQL query text * cleanup * Add indent * Update unit tests * Update unit tests, pt 2 * Fix tweaks * Fix merge conflict resolution Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

* Add `focal` build * upgrade codeQL action to v2 * Pin docker image to stretch (github#1324) * pin stretch image * update stretch repo * Update Dockerfile.test * parse binlog timestamp in UTC (github#1322) * upgrade standard library deps (github#1332) * fix: typo (github#1331) Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com> * Remove stale RELEASE_VERSION file (github#1349) Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Force index in unique key range queries (github#1237) * WIP * Pass entire sql.UniqueKey * newline for limit * Rename var --------- Co-authored-by: meiji163 <meiji163@github.com> * Include git commit in version (github#1347) * Include git commit in version Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Make --version output change less-breaking --------- Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Fix tokens in inspector logging (github#1226) * Cleanup whitespace and comments in SQL query text (github#1246) * Cleanup whitespace in SQL query text * cleanup * Add indent * Update unit tests * Update unit tests, pt 2 * Fix tweaks * Fix merge conflict resolution Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix `--version` output (github#1352) * Fix `--version` output Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Always fallback version/commit if undef Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> --------- Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Update to `go1.21` (github#1351) * go1.21 + bullseye Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go1.21 + bullseye pt 2 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * checkout before setup-go Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go fmt Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Use golangci-lint 1.54.2 to support go1.21 Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * stop using io/ioutil to make linter happy Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Fix typo Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Lint Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * revert replica-tests CI to ubuntu 20 due to linker errors Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Update ensure-go-installed * use `ubuntu-latest` for `ci` job Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * stretch -> bullseye Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> --------- Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> * Replace deprecated `github.com/satori/go.uuid` lib (github#1354) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * Add `CODEOWNERS` file to auto-request reviews (github#1350) Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> * go mod tidy * update test * fix lint --------- Signed-off-by: Tim Vaillancourt <tvaillancourt@slack-corp.com> Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com> Co-authored-by: Rashiq <rashiq@users.noreply.github.com> Co-authored-by: meiji163 <meiji163@github.com> Co-authored-by: guangwu <guoguangwu@magic-shield.com> Co-authored-by: Tim Vaillancourt <tim@timvaillancourt.com>

shaohk · 2024-02-26T09:22:13Z

go/logic/applier.go

@@ -1150,7 +1151,7 @@ func (this *Applier) ApplyDMLEventQueries(dmlEvents [](*binlog.BinlogDMLEvent))
 			return err
 		}

-		sessionQuery := "SET SESSION time_zone = '+00:00'"
+		sessionQuery := "SET /* gh-ost */ SESSION time_zone = '+00:00'"


@timvaillancourt Does the time zone set here need to be configured as this.migrationContext.ApplierTimeZone? I noticed that other sessions are using this.migrationContext.ApplierTimeZone for setting the time zone, but here it is set to +00:00. Should this be adjusted?

@shaohk good question, that sounds more correct than +00:00 🤔

Based on the soak testing this logic has seen I don't think this causes an integrity problem, however. I think that may be because binlog row events contain the values from that primary/master (including dates/times), so it probably doesn't matter what timezone the applier session uses here. I believe gh-ost is using the decoded binlog row event values only

Fewer "what if?" questions is a good thing though, so I would still welcome this change if it passes integration testing at GitHub

timvaillancourt added 6 commits January 11, 2023 22:54

Cleanup whitespace in SQL query text

1e1e42d

cleanup

dae0482

Add indent

4042892

Update unit tests

6f1de5d

Update unit tests, pt 2

704c412

Fix tweaks

7766a35

timvaillancourt marked this pull request as ready for review January 11, 2023 22:47

timvaillancourt requested review from dm-2 and rashiq January 11, 2023 22:47

timvaillancourt added enhancement low-risk labels Jan 11, 2023

timvaillancourt changed the title ~~Cleanup whitespace in SQL query text~~ Cleanup whitespace and comments in SQL query text Jan 11, 2023

timvaillancourt added this to the v1.1.7 milestone Dec 7, 2023

timvaillancourt requested a review from meiji163 December 7, 2023 20:55

meiji163 previously approved these changes Dec 8, 2023

View reviewed changes

Merge branch 'master' into fix-sql-whitespace

eac4234

timvaillancourt dismissed meiji163’s stale review via eac4234 December 8, 2023 22:16

timvaillancourt requested a review from meiji163 December 8, 2023 22:17

Fix merge conflict resolution

17e8fba

Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>

meiji163 approved these changes Dec 8, 2023

View reviewed changes

timvaillancourt merged commit 5ebb953 into github:master Dec 8, 2023
7 checks passed

timvaillancourt deleted the fix-sql-whitespace branch December 8, 2023 23:30

shaohk reviewed Feb 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup whitespace and comments in SQL query text #1246

Cleanup whitespace and comments in SQL query text #1246

timvaillancourt commented Jan 11, 2023 •

edited

Loading

meiji163 commented Dec 8, 2023

shaohk Feb 26, 2024 •

edited

Loading

timvaillancourt Feb 26, 2024 •

edited

Loading

Cleanup whitespace and comments in SQL query text #1246

Cleanup whitespace and comments in SQL query text #1246

Conversation

timvaillancourt commented Jan 11, 2023 • edited Loading

Description

meiji163 commented Dec 8, 2023

shaohk Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

timvaillancourt Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

timvaillancourt commented Jan 11, 2023 •

edited

Loading

shaohk Feb 26, 2024 •

edited

Loading

timvaillancourt Feb 26, 2024 •

edited

Loading