Skip to content

[SPARK-42304][SQL] Rename _LEGACY_ERROR_TEMP_2189 to GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION#42706

Closed
valentinp17 wants to merge 2 commits intoapache:masterfrom
valentinp17:spark-42304
Closed

[SPARK-42304][SQL] Rename _LEGACY_ERROR_TEMP_2189 to GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION#42706
valentinp17 wants to merge 2 commits intoapache:masterfrom
valentinp17:spark-42304

Conversation

@valentinp17
Copy link
Contributor

@valentinp17 valentinp17 commented Aug 28, 2023

What changes were proposed in this pull request?

This PR proposes to assign name to _LEGACY_ERROR_TEMP_2189, "GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION".

Why are the changes needed?

Assign proper name to LEGACY_ERROR_TEMP*

Does this PR introduce any user-facing change?

No

How was this patch tested?

./build/sbt "testOnly org.apache.spark.SparkThrowableSuite"

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

No

@valentinp17
Copy link
Contributor Author

@MaxGekk @itholic PTAL when you find some time.

@itholic
Copy link
Contributor

itholic commented Aug 29, 2023

Can we add a dedicated test for this error class??

},
"UNSUPPORTED_FUNCTION_BY_HIVE_VERSION" : {
"message" : [
"Hive 2.2 and lower versions don't support getTablesByType. Please use Hive 2.3 or higher version."
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the getTablesByType only function that is not supported by Hive 2.2 and lower?? Otherwise I feel the "FUNCTION" in the error class name might too generic.

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 guess I agree.
I can rename to "GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION" if it's not too long

@valentinp17
Copy link
Contributor Author

Can we add a dedicated test for this error class??

I will try to do it

I think this test should cover getTablesByType()

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Thank you, @valentinp17 and @itholic .
Merged to master for Apache Spark 4.0.0.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-42304][SQL] Assign name to _LEGACY_ERROR_TEMP_2189 [SPARK-42304][SQL] Rename _LEGACY_ERROR_TEMP_2189 to GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION Aug 30, 2023
@dongjoon-hyun
Copy link
Member

Do you have ASF JIRA account, @valentinp17 ?

I can assign https://issues.apache.org/jira/browse/SPARK-42304 to you if you have the ID.

@valentinp17
Copy link
Contributor Author

Do you have ASF JIRA account, @valentinp17 ?

I can assign https://issues.apache.org/jira/browse/SPARK-42304 to you if you have the ID.

Not yet. I sent a request to create ASF JIRA account.
Can I tag you here to assign this Jira ticket later?

@valentinp17
Copy link
Contributor Author

Thank you, @dongjoon-hyun
I created ASF JIRA account. Jira username: valentinp17

@dongjoon-hyun
Copy link
Member

I added to you the Apache Spark contributor group and assigned SPARK-42304 to you.
Welcome to the Apache Spark community, @valentinp17 !

@itholic
Copy link
Contributor

itholic commented Aug 30, 2023

Welcome, @valentinp17 !

@itholic
Copy link
Contributor

itholic commented Sep 1, 2023

I will try to do it
I think this test should cover getTablesByType()

Btw, I believe you can add a test as your next work if you're interested in! :-)

@valentinp17 valentinp17 deleted the spark-42304 branch September 9, 2023 18:46
@valentinp17
Copy link
Contributor Author

@itholic Hi!
Of course, I'm interested in!
I'm unsure whether I should create a new Jira for these changes or not. So I decided to show tests in my fork repo first.
PTAL valentinp17@d44216b

cc @MaxGekk @cxzl25

@valentinp17
Copy link
Contributor Author

Additional observation:

Pull request template message looks outdated:

  1. If you want to add or modify an error type or message, please read the guideline first in 'core/src/main/resources/error/README.md'

I suppose it should link to 'common/utils/src/main/resources/error/README.md' instead. If I'm right, I can create PR for this too.

@itholic
Copy link
Contributor

itholic commented Sep 11, 2023

I'm unsure whether I should create a new Jira for these changes or not

We can just reuse the existing JIRA, or we can create a new one. This usually depends on the size of the task, but it's totally up to you :-).

In case reusing the existing JIRA, we typically add a [FOLLOWUP] tag, such as "[SPARK-42304][FOLLOWUP][SQL] Add test for GET_TABLES_BY_TYPE_UNSUPPORTED_BY_HIVE_VERSION"

I suppose it should link to 'common/utils/src/main/resources/error/README.md' instead. If I'm right, I can create PR for this too.

Yeah, seems the link is broken. Let's fix it!

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

Comments