Copilot commented on code in PR #6379:
URL: https://github.com/apache/hive/pull/6379#discussion_r2974542996
##########
llap-common/src/protobuf/LlapDaemonProtocol.proto:
##########
@@ -233,10 +233,11 @@ message SetCapacityRequestProto {
message SetCapacityResponseProto {
}
-// Used for proactive eviction request. Must contain one DB name, and
optionally table information.
+// Used for proactive eviction request. Must contain a DB name, catalog name,
and optionally table information.
message EvictEntityRequestProto {
required string db_name = 1;
repeated TableProto table = 2;
+ required string catalog_name = 3;
Review Comment:
`catalog_name` was added as a **required** field. In protobuf v2, adding a
required field is wire-incompatible: older clients/daemons that send/receive
EvictEntityRequestProto without this field will fail to parse/build the
message. Make this field `optional` (or provide a default) and update
server-side handling to fall back to the default catalog when the field is
absent.
```suggestion
optional string catalog_name = 3;
```
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java:
##########
@@ -199,6 +207,14 @@ public int getBucketingVersion() {
properties.getProperty(hive_metastoreConstants.TABLE_BUCKETING_VERSION));
}
+ public String getCatalogName() {
+ return catalogName;
+ }
+
+ public void setCatalogName(String catalogName) {
+ this.catalogName = catalogName;
Review Comment:
`setCatalogName` allows setting the field to `null`, while the constructor
tries to keep `catalogName` non-null. This can reintroduce nullability and make
callers handle both cases inconsistently. Consider applying the same defaulting
logic in the setter (or clearly documenting/annotating that null is allowed and
ensuring all consumers handle it).
```suggestion
this.catalogName = catalogName == null ? Warehouse.DEFAULT_CATALOG_NAME
: catalogName;
```
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java:
##########
@@ -52,21 +53,28 @@ public class TableDesc implements Serializable, Cloneable {
public static final String SECRET_PREFIX = "TABLE_SECRET";
public static final String SECRET_DELIMIT = "#";
+ private String catalogName;
+
public TableDesc() {
}
/**
* @param inputFormatClass
* @param outputFormatClass
* @param properties must contain serde class name associate with this table.
+ * @param catalogName the catalog this table belongs to; stored as a
dedicated field so it does
+ * not appear in EXPLAIN output. Pass {@code null} for
internal/intermediate
+ * descriptors that are not backed by a real user table.
*/
public TableDesc(
final Class<? extends InputFormat> inputFormatClass,
- final Class<?> outputFormatClass, final Properties properties) {
+ final Class<?> outputFormatClass, final Properties properties,
+ final String catalogName) {
this.inputFileFormatClass = inputFormatClass;
outputFileFormatClass = HiveFileFormatUtils
.getOutputFormatSubstitute(outputFormatClass);
setProperties(properties);
+ this.catalogName = catalogName == null ? Warehouse.DEFAULT_CATALOG_NAME :
catalogName;
}
Review Comment:
Changing the public `TableDesc` constructor signature (adding `catalogName`)
is source-incompatible for any downstream code that instantiates `TableDesc`.
Consider keeping the old 3-arg constructor and delegating to the new one
(passing `null`/default), so external integrations continue to compile.
##########
llap-server/src/java/org/apache/hadoop/hive/llap/io/api/impl/LlapCacheMetadataSerializer.java:
##########
@@ -168,8 +169,14 @@ private static DiskRangeList
decodeRanges(List<LlapDaemonProtocolProtos.CacheEnt
}
private static CacheTag decodeCacheTag(LlapDaemonProtocolProtos.CacheTag ct)
{
- return ct.getPartitionDescCount() == 0 ? CacheTag.build(ct.getTableName())
: CacheTag
- .build(ct.getTableName(), ct.getPartitionDescList());
+ String tableName = ct.getTableName();
+ if (tableName.indexOf('.') == tableName.lastIndexOf('.')) {
+ // In case the CacheTag's table name does not contain catalog name
Review Comment:
The catalog-prefix fallback treats both 1-dot ("db.table") and 0-dot
("table") names as missing catalog because `indexOf('.') == lastIndexOf('.')`
is also true when there is no dot (both -1). That will turn a legacy "table"
into "hive.table" (still not a valid catalog.db.table). Consider checking for
*exactly one* dot (firstDot >= 0 && firstDot == lastDot) and leaving 0-dot
names unchanged or mapping them to a safe default format explicitly.
```suggestion
int firstDot = tableName.indexOf('.');
int lastDot = tableName.lastIndexOf('.');
if (firstDot >= 0 && firstDot == lastDot) {
// In case the CacheTag's table name does not contain catalog name but
has a single db.table component
```
##########
ql/src/java/org/apache/hadoop/hive/llap/ProactiveEviction.java:
##########
@@ -302,45 +334,48 @@ public static Builder create() {
return new Builder();
}
- public Builder addPartitionOfATable(String db, String tableName,
LinkedHashMap<String, String> partSpec) {
- ensureDb(db);
- ensureTable(db, tableName);
- entities.get(db).get(tableName).add(partSpec);
+ /**
+ * Add a partition of a table scoped to the given catalog.
+ */
+ public Builder addPartitionOfATable(String catalog, String db, String
tableName,
+ LinkedHashMap<String, String> partSpec) {
+ ensureTable(catalog, db, tableName);
+ entities.get(catalog).get(db).get(tableName).add(partSpec);
return this;
Review Comment:
`Request.Builder` claims the catalog key defaults to
`Warehouse.DEFAULT_CATALOG_NAME`, but the builder currently stores the
`catalog` parameter as-is. If a caller passes `null` (or an empty string), this
will create a null key and later NPE in `toProtoRequests()` when calling
`toLowerCase()`. Normalize `catalog` (and arguably `db`/`table`) at the builder
boundary, e.g. default null/blank catalog to the default catalog and enforce
non-null keys.
##########
ql/src/java/org/apache/hadoop/hive/ql/plan/TableDesc.java:
##########
@@ -52,21 +53,28 @@ public class TableDesc implements Serializable, Cloneable {
public static final String SECRET_PREFIX = "TABLE_SECRET";
public static final String SECRET_DELIMIT = "#";
+ private String catalogName;
+
public TableDesc() {
}
/**
* @param inputFormatClass
* @param outputFormatClass
* @param properties must contain serde class name associate with this table.
+ * @param catalogName the catalog this table belongs to; stored as a
dedicated field so it does
+ * not appear in EXPLAIN output. Pass {@code null} for
internal/intermediate
+ * descriptors that are not backed by a real user table.
Review Comment:
The javadoc says to pass `null` for internal/intermediate descriptors, but
the constructor always rewrites `null` to `Warehouse.DEFAULT_CATALOG_NAME`.
Either update the javadoc to reflect that `null` means "default catalog" or
preserve `null` in the field and apply the default only in a getter/fallback
method.
```suggestion
* descriptors that are not backed by a real user
table; {@code null} will be
* normalized to {@link Warehouse#DEFAULT_CATALOG_NAME}.
```
--
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]
