You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2024/03/19 10:21:51 UTC
[PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
yaooqinn opened a new pull request, #45588:
URL: https://github.com/apache/spark/pull/45588
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Align mappings of other unsigned numeric types with TINYINT in MySQLDialect. TINYINT is mapping to ByteType and TINYINT UNSIGNED is mapping to ShortType.
In this PR, we
- map SMALLINT to ShortType, SMALLINT UNSIGNED to IntegerType. W/o this, both of them are mapping to IntegerType
- map MEDIUMINT UNSIGNED to IntegerType, and MEDIUMINT is AS-IS. W/o this, MEDIUMINT UNSIGNED uses LongType
Other unsigned/signed types remain unchanged and only improve the test coverage.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Consistency and efficiency while reading MySQL numeric values
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
yes, the mappings described the 1st section.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
new tests
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->no
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2009736101
cc @cloud-fan and @HyukjinKwon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2009732744
So,
- Before: SIGNED SMALLINT(DB) -> SIGNED INT(SPARK) -> SIGNED INT(DB)?
- After: SIGNED SMALLINT(DB) -> SIGNED SHORT(SPARK) -> SIGNED SAMLLINT(DB)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2014421017
Oh, @yaooqinn .
SPARK-47462 is not in branch-3.5, right?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2010001602
Thank you, @yaooqinn and @cloud-fan .
Merged to master for Apache Spark 4.0.0.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2008579737
Hi @yaooqinn
The regression you mentioned was performed in SPARK-43049 and undone in SPARK-46478. SPARK-43049 modified the `string->varchar(255)` to `string->clob` in the Oracle write path to accommodate sufficient character-length. Regrettably, the modification has caused a decline in performance.
In this PR, the changes happen in the read path. The table schema change at the spark side can happen when users perform CTAS against MySQL, i.e. `CREATE TABLE abc AS SELECT * FROM a_jdbc_table`. The table `abc` will result in a schema change after this PR.
It's important to keep in mind that the results of arithmetic operations can differ based on the type of data that is returned.
Since SPARK-45561 already had such impacts for TINYINT in Spark 3.5.1, it seems okay to extend to other types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #45588: [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect
URL: https://github.com/apache/spark/pull/45588
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2011059165
Thank you, @dongjoon-hyun and @cloud-fan.
I will send followups for migration guides
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2008617318
Is this correct?
> Before, we read a smallint(db) as int(spark) in getCatalytType, and then we write an IntegerType(spark) to smallint(db) in getJDBCType
According to `getCommonJDBCType`, `IntegerType(spark)` seems to go `java.sql.Types.INTEGER` instead of `java.sql.Types.SMALLINT`? Maybe, did I miss something in MySQLDialect?
https://github.com/apache/spark/blob/61d7b0f24fc9699d71d0f0c4ca7c162ea55507ea/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala#L142-L146
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2008641864
It's incorrect, it's like we read a smallint and write an int back.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2008586669
No, it was a slightly different issue. IIRC, a user read and tries to write back (with overwrite) and it broke their existing Database schema. And, their whole backend systems was screwed, @yaooqinn .
Maybe, we had better a legacy configuration for this kind of potential schema change stuff.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-47462][SQL] Align mappings of other unsigned numeric types with TINYINT in MySQLDialect [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45588:
URL: https://github.com/apache/spark/pull/45588#issuecomment-2008601011
Thank you @dongjoon-hyun
For the case that users read/write things in a roundtrip:
- Before, we read a smallint(db) as int(spark) in getCatalytType, and then we write an IntegerType(spark) to smallint(db) in getJDBCType
- After, we read a smallint(db) as short(spark) in getCatalytType, and then we write an ShortType(spark) to smallint(db) in getJDBCType
I'm not sure the existing behavior works well but seems a bug to me, and we don't have test cases for that
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org