Fokko opened a new pull request #26737: [SPARK-30103][SQL] Consolidate Schema merge logic URL: https://github.com/apache/spark/pull/26737 While working at https://github.com/apache/spark/pull/26644 I've noticed something strange in behavior. https://github.com/apache/spark/pull/26644 focusses on merging UserDefinedTypes into Spark's native types. Delta checks if the schema is still compatible, so as an integration test I tried to union two DF's, where one has a UserDefinedTypes, which should then be merged into a native type. To mimic this, I've used an union there, because we don't have the Delta extension, and Spark does not check schema compatibility on write, so it is impossible to reproduce the situation that we've observed with Delta. However, when Delta checks compatibility, it merges the schema using the `StructType.merge()`, and when Spark checks compatibility, it uses the `TypeCoercion`: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala#L314-L323 Since this is complex code, I think we should merge these two to get similar behavior. Curious what your opinion is on this, and why these so similar functions are kept separately. ### What changes were proposed in this pull request? Removing the `StructType.merge()`, and use `TypeCoercion.findTightestCommonType()` instead. The one in TypeCoercion looks more complete. <!-- 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. --> ### Why are the changes needed? To simplify the codebase, and consolidate the behavior of merging schemas. <!-- 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. --> ### Does this PR introduce any user-facing change? Not in UI/Console, possibly in behavior. <!-- 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 no, write 'No'. --> ### How was this patch tested? Existing unit tests, <!-- 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. -->
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
