Enable using column identifiers with special characters when deleting table column statistics. #6149
Conversation
zabetak
left a comment
zabetak
left a comment
There was a problem hiding this comment.
Changes LTGM, just optional nits.
|
|
||
| public void lockDbTable(String tableName) throws MetaException { | ||
| if (!ALLOWED_TABLES_TO_LOCK.contains(tableName)) { | ||
| throw new MetaException("Error while locking table " + tableName); |
There was a problem hiding this comment.
Let's add a test case for the exception if its easy/possible.
There was a problem hiding this comment.
I want a Java comment because I can't deserialize the intention about why we should throw an exception.
There was a problem hiding this comment.
Added test case and comment
|
|
||
| try (QueryWrapper queryParams = new QueryWrapper(pm.newQuery("javax.jdo.query.SQL", deleteSql))) { | ||
| executeWithArray(queryParams.getInnerQuery(), params.toArray(), deleteSql); | ||
| } catch (MetaException e) { |
There was a problem hiding this comment.
Catching a MetaException and rethrowing it as a MetaException seems redundant and will make the stacktrace harder to follow. Since the method already throws MetaException can we simply remove the catch block.
okumin
left a comment
okumin
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you
| if (engine != null) { | ||
| deleteSql += " and \"ENGINE\" = '" + engine + "'"; | ||
| deleteSql += " and \"ENGINE\" = ?"; | ||
| params.add(engine); |
There was a problem hiding this comment.
Can we add a test case to add a harmful value?
There was a problem hiding this comment.
added test case to test engine with special character ' in it's name
| List<Long> partitionIds = getPartitionIdsViaSqlFilter(catName, dbName, tblName, sqlFilter, | ||
| input, Collections.emptyList(), -1); | ||
| if (!partitionIds.isEmpty()) { | ||
| String deleteSql = "delete from " + PART_COL_STATS + " where \"PART_ID\" in ( " + getIdListForIn(partitionIds) + ")"; |
There was a problem hiding this 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| public void lockDbTable(String tableName) throws MetaException { | ||
| if (!ALLOWED_TABLES_TO_LOCK.contains(tableName)) { | ||
| throw new MetaException("Error while locking table " + tableName); |
There was a problem hiding this comment.
I want a Java comment because I can't deserialize the intention about why we should throw an exception.
asf-ci-hive
added
tests pending
tests passed
and removed
tests unstable
tests pending
labels
kasakrisz
force-pushed
the
master-del-col-stats
branch
from
2a3154d to
a4be9ad
Compare
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
|
|
is there a JIRA id? if yes, could you please manually link this PR |
6 participants
What changes were proposed in this pull request?
Support deleting table and partition column statistics when column name has special chatacters.
Why are the changes needed?
Special characters like
'are allowed in column names but this character has meaning in SQL language too.Does this PR introduce any user-facing change?
No.
How was this patch tested?