okumin commented on code in PR #6149:
URL: https://github.com/apache/hive/pull/6149#discussion_r2464435266
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -3203,6 +3204,10 @@ private void getStatsTableListResult(
}
public void lockDbTable(String tableName) throws MetaException {
+ if (!ALLOWED_TABLES_TO_LOCK.contains(tableName)) {
+ throw new MetaException("Error while locking table " + tableName);
Review Comment:
I want a Java comment because I can't deserialize the intention about why we
should throw an exception.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -3243,19 +3248,26 @@ public void deleteColumnStatsState(long tbl_id) throws
MetaException {
}
public boolean deleteTableColumnStatistics(long tableId, List<String>
colNames, String engine) {
- String deleteSql = "delete from " + TAB_COL_STATS + " where \"TBL_ID\" = "
+ tableId;
+ String deleteSql = "delete from " + TAB_COL_STATS + " where \"TBL_ID\" =
?";
+ List<Object> params = new ArrayList<>(colNames == null ? 2 :
colNames.size() + 2);
+ params.add(tableId);
+
if (colNames != null && !colNames.isEmpty()) {
- deleteSql += " and \"COLUMN_NAME\" in (" + colNames.stream().map(col ->
"'" + col + "'").collect(Collectors.joining(",")) + ")";
+ deleteSql += " and \"COLUMN_NAME\" in (" + makeParams(colNames.size()) +
")";
+ params.addAll(colNames);
}
+
if (engine != null) {
- deleteSql += " and \"ENGINE\" = '" + engine + "'";
+ deleteSql += " and \"ENGINE\" = ?";
+ params.add(engine);
Review Comment:
Can we add a test case to add a harmful value?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java:
##########
@@ -3269,16 +3281,21 @@ public List<Void> run(List<String> input) throws
Exception {
input, Collections.emptyList(), -1);
if (!partitionIds.isEmpty()) {
String deleteSql = "delete from " + PART_COL_STATS + " where
\"PART_ID\" in ( " + getIdListForIn(partitionIds) + ")";
Review Comment:
This is just a pure question: I don't request any change in this pull
request. Can we potentially replace `getIdListForIn` with a placeholder in this
class at some point? In my opinion, it is better to avoid string concatenation
thoroughly. Otherwise, new issues might easily pass code reviews.
--
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]
