BigQuery: Add support for historical property names for backward compatibility#15991
BigQuery: Add support for historical property names for backward compatibility#15991dbjnbnrj wants to merge 1 commit intoapache:mainfrom
Conversation
dbjnbnrj
force-pushed
the
historical-properties
branch
from
6533889 to
00c9baa
Compare
dbjnbnrj
changed the title
gaborkaszab
left a comment
gaborkaszab
left a comment
There was a problem hiding this comment.
Hi @dbjnbnrj , Thank you for submitting the PR!
I'm pretty hesitant to add something to OSS Iceberg that is to cover some proprietary internal mechanism TBH. As you write in HistoricalPropertyNames these params are to fallback to some Google internal stuff, I don't see much benefit of adding these to open source.
Let's see what others think.
| + "Found both properties '%s' and '%s', only one must be used.", | ||
| PROJECT_ID, | ||
| HistoricalPropertyNames.PROJECT_ID); | ||
| Preconditions.checkArgument( |
There was a problem hiding this comment.
Probably no need for this check because seem to overlap with the one at L129
| projectId != null, "Invalid GCP project: %s must be specified", PROJECT_ID); | ||
|
|
||
| this.location = properties.getOrDefault(GCP_LOCATION, DEFAULT_GCP_LOCATION); | ||
| String fallbackLocation = |
There was a problem hiding this comment.
this.location could be populated the same way as this.projectId by using a ternary operator to handle the fallback.
You might want to consider using PropertyUtil too:
PropertyUtil.propertyAsString(
mergedProps,
RESTCatalogProperties.NAMESPACE_SEPARATOR,
RESTUtil.NAMESPACE_SEPARATOR_URLENCODED_UTF_8);
Thanks for the review @gaborkaszab . Wanted to confirm with @vladislav-sidorovich and @talatuyarer if this change is needed. Its a fix for #14422. |
2 participants
Add support for historical property names (
gcp_projectandgcp_location) inBigQueryPropertiesto maintain backward compatibility with internal configurations.Fixes #14422