Copilot commented on code in PR #11470:
URL: https://github.com/apache/gravitino/pull/11470#discussion_r3370582055
##########
web-v2/web/src/config/catalog.js:
##########
@@ -282,6 +349,62 @@ export const rangerDefaultProps = [
]
export const providerBase = {
+ glue: {
+ label: 'AWS Glue',
+ defaultProps: [
+ {
+ label: 'AWS Region',
+ key: 'aws-region',
+ value: '',
+ required: true,
+ description: 'AWS region, e.g. us-east-1'
+ },
+ {
+ label: 'Warehouse',
+ key: 'warehouse',
+ value: '',
+ required: true,
+ description: 'S3 base path, e.g. s3://my-bucket/warehouse'
+ },
+ {
+ label: 'AWS Glue Catalog ID',
+ key: 'aws-glue-catalog-id',
+ value: '',
+ description: "12-digit AWS account ID; defaults to caller's account"
+ },
+ {
+ label: 'AWS Access Key ID',
+ key: 'aws-access-key-id',
+ value: '',
+ description: 'Static credential; omit to use default credential chain'
+ },
+ {
+ label: 'AWS Secret Access Key',
+ key: 'aws-secret-access-key',
+ value: '',
+ description: 'Static credential paired with aws-access-key-id'
+ },
+ {
+ label: 'AWS Glue Endpoint',
+ key: 'aws-glue-endpoint',
+ value: '',
+ description: 'Custom endpoint URL, e.g. http://localhost:4566 for
LocalStack'
+ },
+ {
+ label: 'Default Table Format',
+ key: 'default-table-format',
+ value: 'hive',
+ select: ['hive', 'iceberg'],
+ description: 'Default format for createTable()'
+ },
+ {
+ label: 'Table Format Filter',
+ key: 'table-format-filter',
+ value: 'all',
+ description: 'Comma-separated formats exposed by listTables()'
+ }
+ ]
Review Comment:
This PR’s title/description focus on normalizing an empty
`aws-glue-catalog-id` and claim “No” user-facing changes, but this file adds a
new Glue provider (labels, default properties, and provider list entry) which
is user-visible UI behavior. Consider splitting the UI/provider additions into
a separate PR, or update the PR title/description and user-facing-change answer
to reflect the added Glue UI support.
##########
web-v2/web/src/components/Icons.js:
##########
@@ -153,6 +153,22 @@ const Icons = {
</g>
</svg>
),
+ glue: props => (
+ <svg {...props} xmlns='http://www.w3.org/2000/svg' width='1em'
height='1em' viewBox='0 0 256 256'>
+ <path d='M0 0h256v256H0z' fill='none' />
+ <defs>
+ <linearGradient id='SVGusebDeuy' x1='0%' x2='100%' y1='100%' y2='0%'>
+ <stop offset='0%' stopColor='#4d27a8' />
+ <stop offset='100%' stopColor='#a166ff' />
+ </linearGradient>
+ </defs>
+ <path fill='url(#SVGusebDeuy)' d='M0 0h256v256H0z' />
+ <path
+ fill='#fff'
+ d='M105.6 73.6h6.4v-6.4h-6.4zm-6.4 3.2V64a3.2 3.2 0 0 1
3.2-3.2h12.8a3.2 3.2 0 0 1 3.2 3.2v12.8a3.2 3.2 0 0 1-3.2 3.2h-12.8a3.2 3.2 0 0
1-3.2-3.2m32-22.4h6.4V48h-6.4zm-6.4 3.2V44.8a3.2 3.2 0 0 1 3.2-3.2h12.8a3.2 3.2
0 0 1 3.2 3.2v12.8a3.2 3.2 0 0 1-3.2 3.2H128a3.2 3.2 0 0 1-3.2-3.2m12.8
28.8h6.4V80h-6.4zm-3.2-12.8h12.8a3.2 3.2 0 0 1 3.2 3.2v12.8a3.2 3.2 0 0 1-3.2
3.2h-12.8a3.2 3.2 0 0 1-3.2-3.2V76.8a3.2 3.2 0 0 1 3.2-3.2m-6.4 33.6c-5.635
0-26.067-.24-39.357-3.507l32.464 51.398c.32.512.493 1.104.493 1.709v7.286c4.493
1.303 8.643 1.316
12.8.052V156.8c0-.605.173-1.197.493-1.709l32.464-51.398c-13.29 3.267-33.722
3.507-39.357 3.507m-50.387-8.986c-.509-.742-.813-1.542-.813-2.416c0-9.104
30.029-10.787 47.93-10.982l.067 6.4c-21.99.24-35.427 2.749-40.163 4.618c5.024
2.099 19.824 4.966 43.366 4.966c23.482 0 38.262-2.851
43.325-4.947c-2.067-.835-6.349-2.07-14.912-3.117l.774-6.352c19.399 2.368 22.013
6.086 22.013 9.414c0 .874-.304 1.674-.813 2.416L140.8 157.728v8.672a3.2 3.2 0 0
1-2.006
2.97c-3.492 1.401-6.999 2.099-10.583 2.099c-3.552
0-7.174-.688-10.918-2.067a3.2 3.2 0 0 1-2.093-3.002v-8.672zm72.544 92.56a3.2
3.2 0 0 1-.695 3.488l-19.2 19.2a3.2 3.2 0 0 1-2.262.938a3.2 3.2 0 0
1-2.262-.938l-19.2-19.2a3.2 3.2 0 0 1 2.262-5.462h9.6v-9.6h6.4V192a3.2 3.2 0 0
1-3.2 3.2h-5.075L128 206.675l11.475-11.475H134.4a3.2 3.2 0 0
1-3.2-3.2v-12.8h6.4v9.6h9.6c1.293 0 2.464.78 2.957 1.974'
+ />
+ </svg>
+ ),
Review Comment:
The embedded SVG defines a hard-coded gradient id (`id='SVGusebDeuy'`).
Rendering this icon multiple times on the same page produces duplicate DOM ids,
which is invalid HTML/SVG and can cause gradient resolution issues if another
SVG asset reuses the same id. Generate a per-instance id (e.g., via
`React.useId()`) and reference it in the `url(#...)` fill.
##########
catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java:
##########
@@ -134,7 +134,8 @@ public void initialize(
Map<String, String> config, CatalogInfo info, HasPropertyMetadata
propertiesMetadata)
throws RuntimeException {
this.glueClient = GlueClientProvider.buildClient(config);
- this.catalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ String rawCatalogId = config.get(GlueConstants.AWS_GLUE_CATALOG_ID);
+ this.catalogId = (rawCatalogId != null && !rawCatalogId.isEmpty()) ?
rawCatalogId : null;
this.defaultTableFormat =
Review Comment:
The new normalization of an empty `aws-glue-catalog-id` to `null` is the
core bug fix, but there’s no unit test asserting that an empty string is
treated as unset (so Glue requests don’t get `catalogId=""`). Please add a test
that covers this regression case (e.g., by extracting catalog-id normalization
into a small helper method that can be unit-tested, and/or verifying
`applyCatalogId` is not invoked when the configured value is empty).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
