Skip to content

Conversation

itholic
Copy link
Contributor

@itholic itholic commented Oct 25, 2024

What changes were proposed in this pull request?

This PR proposes to assign proper error condition & sqlstate for _LEGACY_ERROR_TEMP_2067: UNSUPPORTED_PARTITION_TRANSFORM

Why are the changes needed?

To improve the error message by assigning proper error condition and SQLSTATE

Does this PR introduce any user-facing change?

No, only user-facing error message improved

How was this patch tested?

Updated the existing tests

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

No

…OR_TEMP_2067: UNSUPPORTED_PARTITION_TRANSFORM
@itholic itholic requested a review from MaxGekk October 25, 2024 10:32
@github-actions github-actions bot added the SQL label Oct 25, 2024
errorClass = "_LEGACY_ERROR_TEMP_2067",
messageParameters = Map("transform" -> transform.toString()))
errorClass = "UNSUPPORTED_PARTITION_TRANSFORM",
messageParameters = Map("transform" -> toSQLId(transform.toString)))
Copy link
Member

Choose a reason for hiding this comment

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

Isn't transform an expression actually. If so, please, use toSQLExpr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it but unfortunately it says Transform is not an Expression:

Screenshot 2024-10-29 at 1 54 45 PM

Is there happen to any way converting Transform to Expression?

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see. Transform extends another Expression from org.apache.spark.sql.connector.expressions. Could you add one more definition of toSQLExpr which accept connector.expressions.Expression. It should quote in the same way as the existing toSQLExpr (using quoteByDefault).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure will add it. Thanks for the comment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, @MaxGekk I just realized during addressing the comment that we also should add all internal functions such as toPrettySQL(e: Expression), usePrettyExpression(e: Expression), Expression.sql, transform, dataType, etc. that are used by quoteByDefault internally.

Screenshot 2024-11-18 at 5 48 28 PM

I believe the number of changes and complexity would not be very trivial so maybe we better open separate ticket and discuss there?

Copy link
Member

Choose a reason for hiding this comment

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

I see. thank you for the investigation.

@MaxGekk
Copy link
Member

MaxGekk commented Nov 21, 2024

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

@MaxGekk MaxGekk closed this in 0f1e410 Nov 21, 2024
@MaxGekk
Copy link
Member

MaxGekk commented Nov 21, 2024

ah, the ticket SPARK-50016 is wrong. @itholic could provide a correct one.

@itholic itholic changed the title [SPARK-50016][SQL] Assign appropriate error condition for _LEGACY_ERROR_TEMP_2067: UNSUPPORTED_PARTITION_TRANSFORM [SPARK-50116][SQL] Assign appropriate error condition for _LEGACY_ERROR_TEMP_2067: UNSUPPORTED_PARTITION_TRANSFORM Nov 25, 2024
@itholic
Copy link
Contributor Author

itholic commented Nov 25, 2024

Oh, it should be SPARK-50116. Let me update the JIRA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants