Skip to content

Comments

[SPARK-44608][SQL] Remove unused definitions from DataTypeExpression#42240

Closed
LuciferYang wants to merge 1 commit intoapache:masterfrom
LuciferYang:SPARK-44608
Closed

[SPARK-44608][SQL] Remove unused definitions from DataTypeExpression#42240
LuciferYang wants to merge 1 commit intoapache:masterfrom
LuciferYang:SPARK-44608

Conversation

@LuciferYang
Copy link
Contributor

What changes were proposed in this pull request?

#41559 have defined some DataTypeExpression but did not use, such as ByteTypeExpression, ShortTypeExpression, etc. This pr cleans them up and we can define them when we really need.

Why are the changes needed?

Code cleanup

Does this PR introduce any user-facing change?

No

How was this patch tested?

Pass GitHub Actions

@github-actions github-actions bot added the SQL label Jul 31, 2023
@LuciferYang LuciferYang marked this pull request as draft July 31, 2023 11:48
@LuciferYang LuciferYang marked this pull request as ready for review August 1, 2023 09:11
@LuciferYang
Copy link
Contributor Author

@amaliujia Can we temporarily delete this useless definitions?

@amaliujia
Copy link
Contributor

@LuciferYang we added this for the completeness of DataTypeExpressions. It is just natural that each data type has its DataTypeExpression.

Does it cause any problem so far?

@LuciferYang
Copy link
Contributor Author

LuciferYang commented Aug 1, 2023

@amaliujia These are some unused and untested code, and they are public. If they are released in Spark 3.5, we have an obligation to continue maintaining them and ensuring compatibility, even if they will never be used, which itself is a risk. So I prefer to define when there are actual needs

Of course, if we are confident that they will be used and tested soon, then I think there is no problem keeping these definitions

@LuciferYang
Copy link
Contributor Author

@amaliujia Let’s keep these definitions, I respect your choice.

@LuciferYang LuciferYang closed this Aug 2, 2023
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