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 RETURNING support for MariaDB 10.5.1+ #201

Merged
merged 2 commits into from
Jan 19, 2024
Merged

Add RETURNING support for MariaDB 10.5.1+ #201

merged 2 commits into from
Jan 19, 2024

Conversation

mirromutth
Copy link
Contributor

Motivation:

Add RETURNING clause support for MariaDB 10.5.1+ .

See also #177 .

Modification:

Add RETURNING clause in MySqlStatement#returnGeneratedValues and query message packets.

Result:

For MariaDB 10.5.1 and above, MySqlStatement#returnGeneratedValues will use RETURNING clause instead of LAST_INSERT_ID.

Notice:

For MariaDB 10.5.0 and below, MySqlStatement#returnGeneratedValues will still use LAST_INSERT_ID due to these versions do not support RETURNING clause.

@mirromutth mirromutth added the enhancement New feature or request label Jan 16, 2024
@mirromutth mirromutth marked this pull request as draft January 16, 2024 03:54
@mirromutth mirromutth force-pushed the feature/issues-177 branch 2 times, most recently from 11e1501 to f2e3c2d Compare January 17, 2024 01:18
@mirromutth mirromutth marked this pull request as ready for review January 17, 2024 01:20
@mirromutth mirromutth linked an issue Jan 17, 2024 that may be closed by this pull request
@mirromutth
Copy link
Contributor Author

mirromutth commented Jan 17, 2024

@jchrys

Currently, it is simplify to add RETURNING following original SQL statement without checking.

Perhaps the statement parser needs to be rewritten to support checking whether the statement supports RETURNING (INSERT/REPLACE/DELETE) or already includes RETURNING, like r2dbc-mariadb does?

@jchrys
Copy link
Collaborator

jchrys commented Jan 17, 2024

@jchrys

Currently, it is simplify to add RETURNING following original SQL statement without checking.

Perhaps the statement parser needs to be rewritten to support checking whether the statement supports RETURNING (INSERT/REPLACE/DELETE) or already includes RETURNING, like r2dbc-mariadb does?

I think the current change is suitable for merging, as MariaDB will naturally return a syntax error in case of incorrect RETURNING usage. I agree that enhancing the parser, akin to what's done in r2dbc-mariadb, better be next step.

Copy link
Collaborator

@jchrys jchrys left a comment

Choose a reason for hiding this comment

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

lgtm 👍

src/main/java/io/asyncer/r2dbc/mysql/MySqlResult.java Outdated Show resolved Hide resolved
@jchrys jchrys added this to the 1.0.7/0.9.8 milestone Jan 17, 2024
@mirromutth mirromutth force-pushed the feature/issues-177 branch 2 times, most recently from 8482577 to b7e2f4a Compare January 18, 2024 01:57
@mirromutth
Copy link
Contributor Author

mirromutth commented Jan 18, 2024

Columns should not be quoted, it can be an expression. e.g. INSERT INTO ... RETURNING CURRENT_TIMESTAMP.

If we quoted it, like in the example above it would be INSERT INTO ... RETURNING `CURRENT_TIMESTAMP`, which would be confusing. User wants the result of the CURRENT_TIMESTAMP expression, not a column named CURRENT_TIMESTAMP.

Keep columns raw and let ORM/QueryBuilder handle it.

@mirromutth
Copy link
Contributor Author

@jchrys It is foreseeable that this PR will change the behavior of returnGenerateValues when connecting to MariaDB 10.5.1+, maybe we should merge it with the next minor version?

For example, if a user is using returnGenerateValues without arguments, we will return a Result with one and only one row, which contains a LAST_INSERT_ID column. This modification will change the number of result rows (maybe more, maybe 0 by INSERT ... SELECT ... RETURNING ...), and the row metadata will contain all data columns in the table.

@jchrys
Copy link
Collaborator

jchrys commented Jan 18, 2024

@jchrys It is foreseeable that this PR will change the behavior of returnGenerateValues when connecting to MariaDB 10.5.1+, maybe we should merge it with the next minor version?

For example, if a user is using returnGenerateValues without arguments, we will return a Result with one and only one row, which contains a LAST_INSERT_ID column. This modification will change the number of result rows (maybe more, maybe 0 by INSERT ... SELECT ... RETURNING ...), and the row metadata will contain all data columns in the table.

I agree. 😄

@jchrys jchrys modified the milestones: 1.0.7/0.9.8, 1.1.0/0.10.0 Jan 18, 2024
@jchrys jchrys modified the milestones: 1.1.0/0.10.0, 1.0.7/0.9.8 Jan 18, 2024
@jchrys jchrys merged commit 073af3a into trunk Jan 19, 2024
12 checks passed
@jchrys jchrys deleted the feature/issues-177 branch January 19, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support RETURNING for MariaDB
2 participants