Skip to content

Conversation

@Akanksha-kedia
Copy link
Contributor

@Akanksha-kedia Akanksha-kedia commented Oct 17, 2023

No description provided.

@Akanksha-kedia Akanksha-kedia requested a review from a team as a code owner October 17, 2023 11:14
@github-actions
Copy link

github-actions bot commented Oct 17, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff 16970a0...6cd105c.

Notify File(s)
@steveburnett presto-docs/src/main/sphinx/connector/mysql.rst

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

Nice work, thanks! I have a few suggestions, but not many.

@Akanksha-kedia
Copy link
Contributor Author

Nice work, thanks! I have a few suggestions, but not many.

@steveburnett Thanks !!! for review.

Copy link
Contributor

@kiersten-stokes kiersten-stokes left a comment

Choose a reason for hiding this comment

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

Looks good! I have one very minor formatting suggestion below

Copy link
Contributor

Choose a reason for hiding this comment

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

The widths attribute here specifies the relative width of each column rather than some fixed value. I think we'd be safe to remove it (or, put 50, 50). This also has the happy side effect that the unnecessary wrapping of DECIMAL(P, S) in the "PrestoDB to MySQL type mapping" table will no longer occur

Suggested change
:widths: 40, 50
Suggested change
:widths: 40, 50
:widths: 50, 50

Copy link
Contributor

Choose a reason for hiding this comment

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

Same here :)

Suggested change
:widths: 40, 50
Suggested change
:widths: 40, 50
:widths: 50, 50

Copy link
Contributor

@steveburnett steveburnett left a comment

Choose a reason for hiding this comment

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

LGTM! (doc)

@Akanksha-kedia Akanksha-kedia force-pushed the sqltype branch 2 times, most recently from 457ccbf to dfe558e Compare October 19, 2023 11:46
@Akanksha-kedia
Copy link
Contributor Author

Akanksha-kedia commented Oct 20, 2023

@tdcmeehan help to merge this and adding label as "pr bootcamp" what does that mean exactly?

@Akanksha-kedia
Copy link
Contributor Author

@tdcmeehan please help to review and let me know any more checks are to be done, help to merge.

@tdcmeehan
Copy link
Contributor

While I'm supportive of this documentation, can you prove to me (through tests or references to code) that it's an accurate listing of types?

@tdcmeehan tdcmeehan removed their assignment Nov 3, 2023
@Akanksha-kedia
Copy link
Contributor Author

Akanksha-kedia commented Nov 7, 2023

mysql> describe test_table1;
+---------------+----------+------+-----+---------+-------+
| Field | Type | Null | Key | Default | Extra |
+---------------+----------+------+-----+---------+-------+
| col_char | char(10) | YES | | NULL | |
| col_varchar | tinytext | YES | | NULL | |
| col_json | longtext | YES | | NULL | |
| col_date | date | YES | | NULL | |
| col_time | time | YES | | NULL | |
| col_timestamp | datetime | YES | | NULL | |
+---------------+----------+------+-----+---------+-------+

presto:test1> describe test_table1
-> ;
Column | Type | Extra | Comment
---------------+--------------+-------+---------
col_char | char(10) | |
col_varchar | varchar(255) | |
col_json | varchar | |
col_date | date | |
col_time | time | |
col_timestamp | timestamp | |
(6 rows)

Query 20231107_041552_00035_gtq9g, FINISHED, 1 node
Splits: 19 total, 19 done (100.00%)
[Latency: client-side: 0:02, server-side: 0:02] [6 rows, 430B] [3 rows/s, 234B/s]

Screenshot 2023-11-07 at 9 34 26 AM Screenshot 2023-11-07 at 9 34 46 AM

CREATE TABLE test_table (
col_bit BIT,
col_boolean BOOLEAN,
col_tinyint TINYINT,
col_tinyint_unsigned TINYINT UNSIGNED,
col_smallint SMALLINT,
col_smallint_unsigned SMALLINT UNSIGNED,
col_integer INTEGER,
col_integer_unsigned INTEGER UNSIGNED,
col_bigint BIGINT,
col_bigint_unsigned BIGINT UNSIGNED,
col_double DOUBLE PRECISION,
col_float FLOAT,
col_real REAL,
col_decimal DECIMAL(10, 2), -- replace 10, 2 with actual values
col_char CHAR(10), -- replace 10 with actual value
col_varchar VARCHAR(255), -- replace 255 with actual value
col_tinytext TINYTEXT,
col_text TEXT,
col_mediumtext MEDIUMTEXT,
col_longtext LONGTEXT,
col_enum ENUM('value1', 'value2'), -- replace 'value1', 'value2' with actual enum values
col_binary BINARY(255), -- replace 255 with actual value
col_json JSON,
col_date DATE,
col_time TIME(2), -- replace 2 with actual value
col_datetime DATETIME(2), -- replace 2 with actual value
col_timestamp TIMESTAMP(2) -- replace 2 with actual value
);

CREATE TABLE test_table (
col_bit BOOLEAN,
col_boolean TINYINT,
col_tinyint TINYINT,
col_tinyint_unsigned SMALLINT,
col_smallint SMALLINT,
col_smallint_unsigned INTEGER,
col_integer INTEGER,
col_integer_unsigned BIGINT,
col_bigint BIGINT,
col_bigint_unsigned DECIMAL(20, 0),
col_double DOUBLE,
col_float REAL,
col_real REAL,
col_decimal DECIMAL(10, 2), -- replace 10, 2 with actual values
col_char CHAR(10), -- replace 10 with actual value
col_varchar VARCHAR(255), -- replace 255 with actual value
col_tinytext VARCHAR(255),
col_text VARCHAR(65535),
col_mediumtext VARCHAR(16777215),
col_longtext VARCHAR,
col_enum VARCHAR(255), -- replace 255 with actual value
col_binary VARBINARY,
col_json VARCHAR, -- changed from JSON to VARCHAR
col_date DATE,
col_time TIME, -- changed from TIME WITH TIME ZONE to TIME
col_datetime TIMESTAMP, -- changed from TIMESTAMP WITH TIME ZONE to TIMESTAMP
col_timestamp TIMESTAMP -- changed from TIMESTAMP WITH TIME ZONE to TIMESTAMP
);

Screenshot 2023-11-07 at 10 20 53 AM Screenshot 2023-11-07 at 10 21 05 AM

@Akanksha-kedia
Copy link
Contributor Author

@tdcmeehan please review and help to merge.

@Akanksha-kedia
Copy link
Contributor Author

@tdcmeehan please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants