Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: Add GEOMETRY column type #44849

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented Jun 20, 2023

What problem does this PR solve?

Issue Number: ref #6347

Problem Summary:

This adds a new column type: GEOMETRY that is compatible with MySQL.

This stores values in the format used by mysql

This is only the column type and does not yet add any functions for working with these. This is to limit the current scope. This makes it possible to migrate data from MySQL and view this data in tools like DBeaver. This also doesn't add support for SRID constraints yet.

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
  • No code

Side effects

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

Support for the GEOMETRY column type has been added

@ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 20, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@ti-chi-bot ti-chi-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 20, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jun 20, 2023

/test all

@dveeden dveeden requested review from mjonss, hawkingrei and bb7133 June 20, 2023 14:49
@dveeden dveeden force-pushed the geometry_type branch 2 times, most recently from 9f9bd27 to 6d208ea Compare June 20, 2023 14:54
@dveeden
Copy link
Contributor Author

dveeden commented Jun 20, 2023

/retest

@dveeden dveeden force-pushed the geometry_type branch 2 times, most recently from f2fcc61 to 4276847 Compare June 20, 2023 15:19
@dveeden
Copy link
Contributor Author

dveeden commented Jun 20, 2023

/retest

@dveeden
Copy link
Contributor Author

dveeden commented Jun 20, 2023

/retest

@dveeden dveeden marked this pull request as ready for review June 20, 2023 16:05
@ti-chi-bot ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 20, 2023
@hawkingrei
Copy link
Member

/test tiprow_test

@hawkingrei
Copy link
Member

/test tiprow_fast_test

@ti-chi-bot
Copy link

ti-chi-bot bot commented Jun 21, 2023

@hawkingrei: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build
  • /test canary-scan-security
  • /test check-dev
  • /test check-dev2
  • /test mysql-test
  • /test pingcap/tidb/canary_ghpr_unit_test
  • /test pull-integration-br-test
  • /test pull-integration-mysql-test
  • /test unit-test

Use /test all to run the following jobs that were automatically triggered:

  • pingcap/tidb/ghpr_build
  • pingcap/tidb/ghpr_check
  • pingcap/tidb/ghpr_check2
  • pingcap/tidb/ghpr_mysql_test
  • pingcap/tidb/ghpr_unit_test

In response to this:

/test tiprow_fast_test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mjonss mjonss changed the title parser: Add GEOMETRY column type *: Add GEOMETRY column type Jun 21, 2023
Copy link
Contributor

@mjonss mjonss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all the changes in DEPS.bzl, due to new dependencies?

types/datum.go Outdated
Comment on lines 1761 to 1771
fromBinary := d.Collation() == charset.CollationBin
toBinary := target.GetCharset() == charset.CharsetBin
if fromBinary && toBinary {
s = d.GetString()
} else if fromBinary {
s, err = d.GetBinaryStringDecoded(sc, target.GetCharset())
} else if toBinary {
s = d.GetBinaryStringEncoded()
} else {
s, err = d.GetStringWithCheck(sc, target.GetCharset())
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem to be covered by the tests.

types/datum.go Outdated
case KindBinaryLiteral:
s, err = d.GetBinaryStringDecoded(sc, target.GetCharset())
default:
return invalidConv(d, target.GetType())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also not covered

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023
@pingcap pingcap deleted a comment from ti-chi-bot bot Jun 24, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jun 27, 2023

Are all the changes in DEPS.bzl, due to new dependencies?

Yes, I think so. You can check by unstaging the changes to DEPS.bzl and running make bazel_prepare.

$ git show -U0 -- DEPS.bzl | grep -E '^\+\s*importpath'
+        importpath = "github.com/Azure/go-ansiterm",
+        importpath = "github.com/cenkalti/backoff/v4",
+        importpath = "github.com/containerd/continuity",
+        importpath = "github.com/docker/cli",
+        importpath = "github.com/docker/docker",
+        importpath = "github.com/docker/go-connections",
+        importpath = "github.com/google/shlex",
+        importpath = "github.com/Microsoft/go-winio",
+        importpath = "github.com/moby/term",
+        importpath = "github.com/Nvveen/Gotty",
+        importpath = "github.com/opencontainers/go-digest",
+        importpath = "github.com/opencontainers/image-spec",
+        importpath = "github.com/opencontainers/runc",
+        importpath = "github.com/ory/dockertest/v3",
+        importpath = "github.com/twpayne/go-geom",
+        importpath = "github.com/twpayne/go-kml/v2",
$ go mod graph | grep -E '^github.com/twpayne/go-geom'
github.com/twpayne/[email protected] github.com/DATA-DOG/[email protected]
github.com/twpayne/[email protected] github.com/lib/[email protected]
github.com/twpayne/[email protected] github.com/ory/dockertest/[email protected]
github.com/twpayne/[email protected] github.com/stretchr/[email protected]
github.com/twpayne/[email protected] github.com/twpayne/go-kml/[email protected]
github.com/twpayne/[email protected] github.com/Azure/[email protected]
github.com/twpayne/[email protected] github.com/Microsoft/[email protected]
github.com/twpayne/[email protected] github.com/Nvveen/[email protected]
github.com/twpayne/[email protected] github.com/cenkalti/backoff/[email protected]
github.com/twpayne/[email protected] github.com/containerd/[email protected]
github.com/twpayne/[email protected] github.com/davecgh/[email protected]
github.com/twpayne/[email protected] github.com/docker/[email protected]+incompatible
github.com/twpayne/[email protected] github.com/docker/[email protected]+incompatible
github.com/twpayne/[email protected] github.com/docker/[email protected]
github.com/twpayne/[email protected] github.com/docker/[email protected]
github.com/twpayne/[email protected] github.com/gogo/[email protected]
github.com/twpayne/[email protected] github.com/google/[email protected]
github.com/twpayne/[email protected] github.com/imdario/[email protected]
github.com/twpayne/[email protected] github.com/mitchellh/[email protected]
github.com/twpayne/[email protected] github.com/moby/[email protected]
github.com/twpayne/[email protected] github.com/opencontainers/[email protected]
github.com/twpayne/[email protected] github.com/opencontainers/[email protected]
github.com/twpayne/[email protected] github.com/opencontainers/[email protected]
github.com/twpayne/[email protected] github.com/pkg/[email protected]
github.com/twpayne/[email protected] github.com/pmezard/[email protected]
github.com/twpayne/[email protected] github.com/sirupsen/[email protected]
github.com/twpayne/[email protected] github.com/xeipuuv/[email protected]
github.com/twpayne/[email protected] github.com/xeipuuv/[email protected]
github.com/twpayne/[email protected] github.com/xeipuuv/[email protected]
github.com/twpayne/[email protected] golang.org/x/[email protected]
github.com/twpayne/[email protected] golang.org/x/[email protected]
github.com/twpayne/[email protected] gopkg.in/[email protected]
github.com/twpayne/[email protected] gopkg.in/[email protected]

@hawkingrei
Copy link
Member

/ok-to-test

@ti-chi-bot ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Jun 27, 2023
@ti-chi-bot ti-chi-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2023
@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2023
@ti-chi-bot ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 6, 2023
@ti-chi-bot ti-chi-bot bot deleted a comment from ti-chi-bot Jul 11, 2023
@dveeden
Copy link
Contributor Author

dveeden commented Jul 11, 2023

/retest

@hawkingrei
Copy link
Member

/test all

@hawkingrei
Copy link
Member

/retest

@ti-chi-bot ti-chi-bot bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 13, 2023
@ti-chi-bot
Copy link

ti-chi-bot bot commented Jul 18, 2023

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Merging #44849 (3456b68) into master (c0affea) will increase coverage by 2.1939%.
Report is 38 commits behind head on master.
The diff coverage is 87.5000%.

❗ Current head 3456b68 differs from pull request most recent head 1468d65. Consider uploading reports for the commit 1468d65 to get more accurate results

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #44849        +/-   ##
================================================
+ Coverage   71.0442%   73.2382%   +2.1939%     
================================================
  Files          1368       1259       -109     
  Lines        402977     388666     -14311     
================================================
- Hits         286292     284652      -1640     
+ Misses        96744      85765     -10979     
+ Partials      19941      18249      -1692     
Flag Coverage Δ
unit 73.2382% <87.5000%> (+2.1939%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 53.6440% <ø> (-0.3224%) ⬇️
parser 85.0695% <ø> (∅)
br 52.0858% <ø> (-0.8856%) ⬇️

Copy link

ti-chi-bot bot commented Dec 4, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign bb7133, d3hunter, tangenta, yudongusa for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dveeden dveeden marked this pull request as draft December 4, 2023 20:44
@ti-chi-bot ti-chi-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2023
@dveeden dveeden removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2023
Copy link

ti-chi-bot bot commented Dec 8, 2023

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2023
Copy link

tiprow bot commented Feb 27, 2024

@dveeden: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
tiprow_fast_test 1468d65 link true /test fast_test_tiprow

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link

ti-chi-bot bot commented Nov 18, 2024

@dveeden: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
idc-jenkins-ci-tidb/build 1468d65 link true /test build
pull-mysql-client-test 1468d65 link true /test pull-mysql-client-test
idc-jenkins-ci-tidb/check_dev 1468d65 link true /test check-dev
idc-jenkins-ci-tidb/check_dev_2 1468d65 link true /test check-dev2
idc-jenkins-ci-tidb/mysql-test 1468d65 link true /test mysql-test
idc-jenkins-ci-tidb/unit-test 1468d65 link true /test unit-test
pull-integration-ddl-test 1468d65 link true /test pull-integration-ddl-test
pull-br-integration-test 1468d65 link true /test pull-br-integration-test
pull-lightning-integration-test 1468d65 link true /test pull-lightning-integration-test
pull-integration-e2e-test 1468d65 link true /test pull-integration-e2e-test

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a PR is ready to be tested. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants