Copilot commented on code in PR #6379:
URL: https://github.com/apache/hive/pull/6379#discussion_r3045710429
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java:
##########
@@ -199,6 +219,14 @@ public int getBucketingVersion() {
properties.getProperty(hive_metastoreConstants.TABLE_BUCKETING_VERSION));
}
+ public String getCatalogName() {
+ return catalogName;
+ }
+
+ public void setCatalogName(String catalogName) {
+ this.catalogName = catalogName == null ? Warehouse.DEFAULT_CATALOG_NAME :
catalogName;
+ }
Review Comment:
`TableDesc.catalogName` can remain null for instances built via the no-arg
constructor + setters, even though the new constructor/setter normalize null to
`Warehouse.DEFAULT_CATALOG_NAME`. Consider initializing `catalogName` eagerly
(field initializer or in `TableDesc()`) so `getCatalogName()` never returns
null. Also, `equals()`/`hashCode()` currently ignore `catalogName`, which can
cause different-catalog descriptors to compare equal and collide in hash-based
collections; include `catalogName` in both (or document why it must be
excluded).
##########
llap-server/src/test/org/apache/hadoop/hive/llap/cache/TestProactiveEviction.java:
##########
@@ -106,6 +109,149 @@ private static void assertMatchOnTags(Builder
requestBuilder, String expected) {
assertEquals(expected, sb.toString());
}
+ /**
+ * Verifies that passing an explicit catalog produces correct matching via
isTagMatch.
+ * TEST_TAGS all belong to the default catalog, so requests for a different
catalog must not match.
+ */
+ @Test
+ public void testCatalogAwareCacheTagAndRequestMatching() {
+ // Default catalog matches as expected.
+ assertMatchOnTags(Builder.create().addDb("fx"), "111111111111000000");
+ assertMatchOnTags(Builder.create().addTable("fx", "futures"),
"000001111000000000");
+ assertMatchOnTags(Builder.create().addPartitionOfATable("fx", "futures",
+ buildParts("ccy", "JPY")), "000000110000000000");
+ assertMatchOnTags(Builder.create().addTable("fixedincome", "bonds"),
"000000000000000110");
+ assertMatchOnTags(Builder.create().addPartitionOfATable("fx", "rates",
+ buildParts("from", "EUR", "to", "HUF")), "000010000000000000");
+
+ // Non-default catalog: CacheTag now carries catalog info, so none of the
TEST_TAGS
+ // (all default-catalog) should match requests targeting a different
catalog.
+ assertMatchOnTags(Builder.create().addDb("custom_catalog", "fx"),
"000000000000000000");
+ assertMatchOnTags(Builder.create().addTable("custom_catalog", "equity",
"prices"),
+ "000000000000000000");
+ assertMatchOnTags(Builder.create().addPartitionOfATable(
+ "custom_catalog", "equity", "prices", buildParts("ex", "NYSE")),
+ "000000000000000000");
+ }
+
+ /**
+ * Verifies that catalog_name is serialized into the proto and correctly
restored via fromProtoRequest.
+ */
+ @Test
+ public void testProtoRoundTripPreservesCatalog() {
+ // Default catalog is always serialized into the proto.
+ Request defaultCatRequest = Builder.create().addDb("testdb").build();
+ List<EvictEntityRequestProto> protos = defaultCatRequest.toProtoRequests();
+ assertEquals(1, protos.size());
+ EvictEntityRequestProto proto = protos.get(0);
+ assertTrue("catalog_name must be present in proto",
proto.hasCatalogName());
+ assertEquals(Warehouse.DEFAULT_CATALOG_NAME, proto.getCatalogName());
+ assertEquals("testdb", proto.getDbName());
+
+ Request roundTripped = Builder.create().fromProtoRequest(proto).build();
+ assertTrue(roundTripped.hasDatabaseName(Warehouse.DEFAULT_CATALOG_NAME,
"testdb"));
+
+ // Custom catalog is also preserved.
+ Request customCatRequest = Builder.create().addTable("spark_catalog",
"salesdb", "orders").build();
+ protos = customCatRequest.toProtoRequests();
+ assertEquals(1, protos.size());
+ proto = protos.get(0);
+ assertTrue("catalog_name must be present in proto",
proto.hasCatalogName());
+ assertEquals("spark_catalog", proto.getCatalogName());
+ assertEquals("salesdb", proto.getDbName());
+
+ roundTripped = Builder.create().fromProtoRequest(proto).build();
+ assertTrue(roundTripped.hasDatabaseName("spark_catalog", "salesdb"));
+ }
+
+ /**
+ * Verifies that entities in different catalogs are independently scoped
even when they share
+ * the same DB name, and that getSingleCatalogName/getSingleDbName return
null when multiple
+ * catalog-DB pairs are present.
Review Comment:
This test’s Javadoc mentions `getSingleCatalogName/getSingleDbName`, but
those methods no longer exist (they were replaced by
`hasCatalogName/hasDatabaseName`). Update the comment to reflect the current
API/behavior to avoid confusion for future maintainers.
```suggestion
* the same DB name, and that requests spanning multiple catalog-DB pairs
are not treated as
* having a single catalog or database; callers should use
hasCatalogName/hasDatabaseName with
* explicit values instead.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
