Skip to content

Conversation

@itholic
Copy link
Contributor

@itholic itholic commented Feb 5, 2023

What changes were proposed in this pull request?

This PR proposes to assign name to _LEGACY_ERROR_TEMP_2123 and _LEGACY_ERROR_TEMP_2125, "CANNOT_MERGE_INCOMPATIBLE_DATA_TYPE".

Why are the changes needed?

We should assign proper name to LEGACY_ERROR_TEMP*

Does this PR introduce any user-facing change?

No

How was this patch tested?

./build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*

@itholic itholic changed the title [SPARK-42318][SQL] Assign name to _LEGACY_ERROR_TEMP_(2123|2125) [SPARK-42318][SPARK-42319][SQL] Assign name to _LEGACY_ERROR_TEMP_(2123|2125) Feb 5, 2023
@itholic
Copy link
Contributor Author

itholic commented Feb 5, 2023

cc @srielau @MaxGekk @cloud-fan Could you review this error class ticket when you find some time 🙏

Copy link
Contributor

@srielau srielau left a comment

Choose a reason for hiding this comment

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

Can you explain when this error is being raised?
It seems to be related to a data source?
MERGE is a SQL Statement, so the term is very confusing.
Also there is no action associated or any other context.

@itholic
Copy link
Contributor Author

itholic commented Feb 6, 2023

MERGE is a SQL Statement, so the term is very confusing.

I agree with that.
Actually the term MERGE here means that merge function of StructType (StructType.merge).
So, seems like it's StructType specific error message.

Can you explain when this error is being raised?

The StructType.merge requires two StructType to be merged together.
When merging the two StructType, it raises error if both schemas don't match.

Btw, on my second thought, it seems to be a private function so can't be used in user space ??

scala> StructType.merge(left, right)
<console>:29: error: method merge in object StructType cannot be accessed in object org.apache.spark.sql.types.StructType
       StructType.merge(left, right)

cc @MaxGekk @cloud-fan Would you confirm if we should switched this error into internal error ??

@srielau
Copy link
Contributor

srielau commented Feb 6, 2023

If it's an internal error we want to issue an XXK sqlstate. For Analysis I have created XXKD0. Would this be considered analysis?
We can also use XX000 generically.

@MaxGekk
Copy link
Member

MaxGekk commented Feb 7, 2023

cc @MaxGekk @cloud-fan Would you confirm if we should switched this error into internal error ??

Don't think so. The function invokes itself recursively to merge struct fields too. Try to reproduce the error from user space using UNION ALL when some nested fields are not compatible.

@itholic
Copy link
Contributor Author

itholic commented Feb 8, 2023

I see.
Then maybe we can keep the name as is? or anyone have some better idea for naming?
Also just assigned "42825" to sqlState and fixed the tests.

@MaxGekk
Copy link
Member

MaxGekk commented Feb 8, 2023

Then maybe we can keep the name as is?

I agree with that.

Also just assigned "42825" to sqlState and fixed the tests.

@itholic Sure. Please, rebase on the recent master.

@itholic
Copy link
Contributor Author

itholic commented Feb 8, 2023

Rebased, thanks!

@MaxGekk
Copy link
Member

MaxGekk commented Feb 8, 2023

+1, LGTM. Merging to master/3.4.
Thank you, @itholic and @srielau for review.

MaxGekk pushed a commit that referenced this pull request Feb 8, 2023
…23|2125)

### What changes were proposed in this pull request?

This PR proposes to assign name to _LEGACY_ERROR_TEMP_2123 and _LEGACY_ERROR_TEMP_2125, "CANNOT_MERGE_INCOMPATIBLE_DATA_TYPE".

### Why are the changes needed?

We should assign proper name to _LEGACY_ERROR_TEMP_*

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

`./build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*`

Closes #39891 from itholic/LEGACY_2125.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
(cherry picked from commit b11fba0)
Signed-off-by: Max Gekk <max.gekk@gmail.com>
@MaxGekk MaxGekk closed this in b11fba0 Feb 8, 2023
@itholic itholic deleted the LEGACY_2125 branch April 22, 2023 05:47
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
…23|2125)

### What changes were proposed in this pull request?

This PR proposes to assign name to _LEGACY_ERROR_TEMP_2123 and _LEGACY_ERROR_TEMP_2125, "CANNOT_MERGE_INCOMPATIBLE_DATA_TYPE".

### Why are the changes needed?

We should assign proper name to _LEGACY_ERROR_TEMP_*

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

`./build/sbt "sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*`

Closes apache#39891 from itholic/LEGACY_2125.

Authored-by: itholic <haejoon.lee@databricks.com>
Signed-off-by: Max Gekk <max.gekk@gmail.com>
(cherry picked from commit b11fba0)
Signed-off-by: Max Gekk <max.gekk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants