-
Notifications
You must be signed in to change notification settings - Fork 212
Upgrade Hive 1.2.2 → 2.3.9 #585
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /** | ||
| * Copyright 2022-2024 LinkedIn Corporation. All rights reserved. | ||
| * Copyright 2022-2026 LinkedIn Corporation. All rights reserved. | ||
| * Licensed under the BSD-2 Clause license. | ||
| * See LICENSE in the project root for license information. | ||
| */ | ||
|
|
@@ -97,6 +97,6 @@ private static IMetaStoreClient getRemoteMetastoreClient(Properties props) | |
| UserGroupInformation.setConfiguration(conf); | ||
| UserGroupInformation.loginUserFromKeytab(clientPrincipal, clientKeytab); | ||
| } | ||
| return RetryingMetaStoreClient.getProxy(conf); | ||
| return RetryingMetaStoreClient.getProxy(conf, true); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is the new parameter in this API and what is the behavior?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question. The new second arg is Hive 2.3.9 removed the single-arg
I've expanded the "Source fixes" section in the PR summary to include this semantic note, so future readers have the context inline. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| # Version of the produced binaries. | ||
| # The version is inferred by shipkit-auto-version Gradle plugin (https://github.com/shipkit/shipkit-auto-version) | ||
| version=2.3.* | ||
| version=2.4.* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for finding this gap. this requires more visibility & tracking. We need to understand if it will fail any existing production views. can you please create a follow up ticket for identifying blast radius and mitigation strategy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you look at the git blame history for this unit test, you might be able to find out why this feature was needed at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points — addressing both.
Origin (per git blame):
testEnumUnionStringwas added in PR #282 (commit6d6f10e, July 2022) alongside theSchemaUtilities.mergeUnionSchemaenum∪string merge logic.Root cause: The failure is in Hive 2.3.9's
SemanticAnalyzer.UnparseTranslator.addTranslationduringCREATE VIEWparsing — upstream of Coral.mergeUnionSchemaitself is unchanged; queries against already-created views using this pattern still translate correctly.Follow-up ticket: Filing one to track:
CAST(enum AS string)at view-author level, upstream Hive parser fix)testEnumUnionStringonce a fix landsShould this be a GitHub issue on this repo, or our internal tracker? Happy either way — let me know the preference.