Skip to content

Enable using column identifiers with special characters when deleting table column statistics. #6149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 3, 2025
+57 −21
Merged

Enable using column identifiers with special characters when deleting table column statistics. #6149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b2b333
Enable using column identifiers with special characters when deleting…
kasakrisz Oct 21, 2025
ae361bf
address review comments: remove redundant MetaException
kasakrisz Nov 3, 2025
a4be9ad
address review comments: add test when engine has special characters
kasakrisz Nov 3, 2025
7a94344
address review comments: add test when try to lock table which is not…
kasakrisz Nov 3, 2025
f4a0e34
address review comments: add comment
kasakrisz Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 30 additions & 14 deletions ...e/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/MetaStoreDirectSql.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@
*/
class MetaStoreDirectSql {
private static final int NO_BATCHING = -1, DETECT_BATCHING = 0;
private static final Set<String> ALLOWED_TABLES_TO_LOCK = Set.of("NOTIFICATION_SEQUENCE");

private static final Logger LOG = LoggerFactory.getLogger(MetaStoreDirectSql.class);
private final PersistenceManager pm;
Expand Down Expand Up @@ -3203,6 +3204,11 @@ private void getStatsTableListResult(
}

public void lockDbTable(String tableName) throws MetaException {
// Only certain tables are allowed to be locked, and the API should restrict them.
if (!ALLOWED_TABLES_TO_LOCK.contains(tableName)) {
throw new MetaException("Error while locking table " + tableName);
Copy link
Member

@zabetak zabetak Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test case for the exception if its easy/possible.

Copy link
Contributor

@okumin okumin Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want a Java comment because I can't deserialize the intention about why we should throw an exception.

Copy link
Contributor Author

@kasakrisz kasakrisz Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test case and comment

}

String lockCommand = "lock table \"" + tableName + "\" in exclusive mode";
try {
executeNoResult(lockCommand);
Expand Down Expand Up @@ -3243,19 +3249,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);
Copy link
Contributor

@okumin okumin Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test case to add a harmful value?

Copy link
Contributor Author

@kasakrisz kasakrisz Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added test case to test engine with special character ' in it's name

}
try {
executeNoResult(deleteSql);
} catch (SQLException e) {
LOG.warn("Error removing table column stats. ", e);

try (QueryWrapper queryParams = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", deleteSql))) {
executeWithArray(queryParams.getInnerQuery(), params.toArray(), deleteSql);
} catch (MetaException e) {
return false;
}

return true;
}

Expand All @@ -3269,17 +3282,20 @@ 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) + ")";
Copy link
Contributor

@okumin okumin Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@simhadri-g simhadri-g Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the partitionIds is returned as long, and getIdListForIn(partitionIds) essentially calls Long.toString() which should output only digits, therefore no special characters can be introduced here. So, i think getIdListForIn(partitionIds) is safe.

Copy link
Contributor

@okumin okumin Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree getIdListForIn((Collection<Long>) partitionIds) is safe, and we don't need to update it in this patch as a short-term remediation. As a long-term remediation, I'd say we should not have a habit of concatenating strings since a single abuse and careless review can introduce a new vulnerability.

List<Object> params = new ArrayList<>(colNames == null ? 1 : colNames.size() + 1);

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);
}
try {
executeNoResult(deleteSql);
} catch (SQLException e) {
LOG.warn("Error removing partition column stats. ", e);
throw new MetaException("Error removing partition column stats: " + e.getMessage());

try (QueryWrapper queryParams = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", deleteSql))) {
executeWithArray(queryParams.getInnerQuery(), params.toArray(), deleteSql);
}
}
return null;
Expand Down
34 changes: 27 additions & 7 deletions ...tore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/TestObjectStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -919,42 +919,48 @@ public void testTableStatisticsOps() throws Exception {
List<ColumnStatistics> tabColStats;
try (AutoCloseable c = deadline()) {
tabColStats = objectStore.getTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1,
Arrays.asList("test_col1", "test_col2"));
Arrays.asList("test_col1", "test_col' 2"));
}
Assert.assertEquals(0, tabColStats.size());

ColumnStatisticsDesc statsDesc = new ColumnStatisticsDesc(true, DB1, TABLE1);
ColumnStatisticsObj statsObj1 = new ColumnStatisticsObj("test_col1", "int",
new ColumnStatisticsData(ColumnStatisticsData._Fields.DECIMAL_STATS, new DecimalColumnStatsData(100, 1000)));
ColumnStatisticsObj statsObj2 = new ColumnStatisticsObj("test_col2", "int",
ColumnStatisticsObj statsObj2 = new ColumnStatisticsObj("test_col' 2", "int",
new ColumnStatisticsData(ColumnStatisticsData._Fields.DECIMAL_STATS, new DecimalColumnStatsData(200, 2000)));
ColumnStatistics colStats = new ColumnStatistics(statsDesc, Arrays.asList(statsObj1, statsObj2));
colStats.setEngine(ENGINE);
objectStore.updateTableColumnStatistics(colStats, null, 0);

try (AutoCloseable c = deadline()) {
tabColStats = objectStore.getTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1,
Arrays.asList("test_col1", "test_col2"));
Arrays.asList("test_col1", "test_col' 2"));
}
Assert.assertEquals(1, tabColStats.size());
Assert.assertEquals(2, tabColStats.get(0).getStatsObjSize());

objectStore.deleteTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1, "test_col1", ENGINE);
try (AutoCloseable c = deadline()) {
tabColStats = objectStore.getTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1,
Arrays.asList("test_col1", "test_col2"));
Arrays.asList("test_col1", "test_col' 2"));
}
Assert.assertEquals(1, tabColStats.size());
Assert.assertEquals(1, tabColStats.get(0).getStatsObjSize());

objectStore.deleteTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1, "test_col2", ENGINE);
objectStore.deleteTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1, "test_col' 2", ENGINE);
try (AutoCloseable c = deadline()) {
tabColStats = objectStore.getTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1,
Arrays.asList("test_col1", "test_col2"));
Arrays.asList("test_col1", "test_col' 2"));
}
Assert.assertEquals(0, tabColStats.size());
}

@Test
public void testDeleteTableColumnStatisticsWhenEngineHasSpecialCharacter() throws Exception {
createPartitionedTable(true, true);
objectStore.deleteTableColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1, "test_col1", "special '");
}

@Test
public void testPartitionStatisticsOps() throws Exception {
createPartitionedTable(true, true);
Expand Down Expand Up @@ -1006,6 +1012,14 @@ public void testPartitionStatisticsOps() throws Exception {
Assert.assertEquals(0, stat.size());
}

@Test
public void testDeletePartitionColumnStatisticsWhenEngineHasSpecialCharacter() throws Exception {
createPartitionedTable(true, true);
objectStore.deletePartitionColumnStatistics(DEFAULT_CATALOG_NAME, DB1, TABLE1,
"test_part_col=a2", List.of("a2"), null, "special '");
}


@Test
public void testAggrStatsUseDB() throws Exception {
Configuration conf2 = MetastoreConf.newMetastoreConf(conf);
Expand Down Expand Up @@ -1051,7 +1065,7 @@ private void createPartitionedTable(boolean withPrivileges, boolean withStatisti
.setDbName(DB1)
.setTableName(TABLE1)
.addCol("test_col1", "int")
.addCol("test_col2", "int")
.addCol("test_col' 2", "int")
.addPartCol("test_part_col", "int")
.addCol("test_bucket_col", "int", "test bucket col comment")
.addCol("test_skewed_col", "int", "test skewed col comment")
Expand Down Expand Up @@ -1239,6 +1253,12 @@ protected Database getJdoResult(ObjectStore.GetHelper<Database> ctx) throws Meta
Assert.assertEquals(1, directSqlErrors.getCount());
}

@Test(expected = MetaException.class)
public void testLockDbTableThrowsExceptionWhenTableIsNotAllowedToLock() throws Exception {
MetaStoreDirectSql metaStoreDirectSql = new MetaStoreDirectSql(objectStore.getPersistenceManager(), conf, null);
metaStoreDirectSql.lockDbTable("TBLS");
}

@Deprecated
private static void dropAllStoreObjects(RawStore store)
throws MetaException, InvalidObjectException, InvalidInputException {
Expand Down