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.

Sign up for GitHub

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

Jump to bottom

[SPARK-47333][SQL] Use checkInputDataTypes to check the parameter types of the function to_xml #45167

Closed
wants to merge 5 commits into from
Closed

[SPARK-47333][SQL] Use checkInputDataTypes to check the parameter types of the function to_xml #45167

wants to merge 5 commits into from

Conversation

panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Feb 19, 2024 •
edited

What changes were proposed in this pull request?

The pr aims to use checkInputDataTypes to check the parameter types of the function to_xml.

Why are the changes needed?

Follow the common pattern for checking input data types in Expression, check the data type during the analysis logic in advance .

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • Add new UT.
  • Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Feb 19, 2024
MaxGekk
MaxGekk reviewed Mar 9, 2024
View reviewed changes
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

@panbingkun Could fill in PR description, please. You changes are pretty useful, I believe.

@panbingkun
Copy link
Contributor Author

@panbingkun Could fill in PR description, please. You changes are pretty useful, I believe.

@MaxGekk

Okay, let me update it.
But this PR actually did two aims, one of which was the deletion of the error classes _LEGACY_ERROR_TEMP_3234, which also involved another PR: #44665.
Because the error classes _LEGACY_ERROR_TEMP_3234 is only used in these two points, as follows:

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/csvExpressions.scala

Line 270 in 76b1c12

errorClass = "_LEGACY_ERROR_TEMP_3234",

spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/xmlExpressions.scala

Line 287 in 76b1c12

errorClass = "_LEGACY_ERROR_TEMP_3234",

I originally wanted to continue this PR after merging 44665 into codebase (so that the error classes _LEGACY_ERROR_TEMP_3234 could be deleted directly).

If you have time, can you help me review this PR 44665 first?

Or I can modify this PR first and temporarily not delete the error class _LEGACY_ERROR_TEMP_3234.

Thank you very much! ❤️

@panbingkun panbingkun changed the title [WIP] Use checkInputDataTypes to check the parameter types of the function to_xml & remove _LEGACY_ERROR_TEMP_3234 [SPARK-47333][SQL] Use checkInputDataTypes to check the parameter types of the function to_xml & remove _LEGACY_ERROR_TEMP_3234 Mar 9, 2024
@MaxGekk
Copy link
Member

MaxGekk commented Mar 10, 2024

If you have time, can you help me review this PR #44665 first?

I will look at it soon. Thanks.

@panbingkun panbingkun changed the title [SPARK-47333][SQL] Use checkInputDataTypes to check the parameter types of the function to_xml & remove _LEGACY_ERROR_TEMP_3234 [SPARK-47333][SQL] Use checkInputDataTypes to check the parameter types of the function to_xml Mar 12, 2024
@panbingkun
Copy link
Contributor Author

panbingkun commented Mar 12, 2024 •
edited

@MaxGekk
Given that 44665 is still under review (there may still be some controversy), let's proceed with this PR first. Currently, I will not delete the error classes _LEGACY_ERROR_TEMP_3234 in this PR, but I will remove it in 44665.
Do you think it is feasible?

@panbingkun panbingkun marked this pull request as ready for review March 12, 2024 06:03
@panbingkun panbingkun requested a review from MaxGekk March 12, 2024 06:03
@MaxGekk
Copy link
Member

MaxGekk commented Mar 13, 2024

+1, LGTM. Merging to master.
Thank you, @panbingkun.

@MaxGekk MaxGekk closed this in c7795bb Mar 13, 2024
sweisdb pushed a commit to sweisdb/spark that referenced this pull request Apr 1, 2024
@panbingkun @sweisdb
[SPARK-47333][SQL] Use checkInputDataTypes to check the parameter typ…
5146d55 
…es of the function `to_xml`

### What changes were proposed in this pull request?
The pr aims to use `checkInputDataTypes` to check the parameter `types` of the function `to_xml`.

### Why are the changes needed?
Follow the common pattern for checking input data types in `Expression`, `check the data type` during the `analysis` logic in advance .

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

### How was this patch tested?
- Add new UT.
- Pass GA.

### Was this patch authored or co-authored using generative AI tooling?
No.

Closes apache#45167 from panbingkun/LEGACY_ERROR_TEMP_3234.

Authored-by: panbingkun <panbingkun@baidu.com>
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
Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
2 participants
@panbingkun @MaxGekk