[
https://issues.apache.org/jira/browse/PHOENIX-6560?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17712934#comment-17712934
]
ASF GitHub Bot commented on PHOENIX-6560:
-----------------------------------------
stoty commented on code in PR #1586:
URL: https://github.com/apache/phoenix/pull/1586#discussion_r1168249389
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -598,18 +599,31 @@ private Job configureJobForPartialBuild() throws
Exception {
}
private long getMaxRebuildAsyncDate(String schemaName, List<String>
disableIndexes) throws SQLException {
- Long maxRebuilAsyncDate=HConstants.LATEST_TIMESTAMP;
- Long maxDisabledTimeStamp=0L;
- if (disableIndexes == null || disableIndexes.isEmpty()) { return
0; }
+ Long maxRebuilAsyncDate = HConstants.LATEST_TIMESTAMP;
+ Long maxDisabledTimeStamp = 0L;
+ if (disableIndexes == null || disableIndexes.isEmpty()) {
+ return 0;
+ }
List<String> quotedIndexes = new
ArrayList<String>(disableIndexes.size());
for (String index : disableIndexes) {
quotedIndexes.add("'" + index + "'");
}
- try (ResultSet rs = connection.createStatement()
- .executeQuery("SELECT MAX(" + ASYNC_REBUILD_TIMESTAMP +
"),MAX("+INDEX_DISABLE_TIMESTAMP+") FROM " + SYSTEM_CATALOG_NAME + " ("
- + ASYNC_REBUILD_TIMESTAMP + " BIGINT) WHERE " +
TABLE_SCHEM
- + (schemaName != null && schemaName.length() > 0 ?
"='" + schemaName + "'" : " IS NULL")
- + " and " + TABLE_NAME + " IN (" +
StringUtils.join(",", quotedIndexes) + ")")) {
+
+ String query = String.format("SELECT MAX(" +
ASYNC_REBUILD_TIMESTAMP + "), "
+ + "MAX(" + INDEX_DISABLE_TIMESTAMP + ") FROM "
+ + SYSTEM_CATALOG_NAME + " (" + ASYNC_REBUILD_TIMESTAMP
+ + " BIGINT) WHERE " + TABLE_SCHEM + " %s AND " +
TABLE_NAME + " IN ( %s )",
+ (schemaName != null && schemaName.length() > 0) ? " =
? " : " IS NULL ",
+ QueryUtil.getDynamicParams(quotedIndexes.size()));
Review Comment:
This name may get confused with the dynamic column feature.
I suggest calling it something like QueryUtil.generateInListParams() or
similar
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -598,18 +599,31 @@ private Job configureJobForPartialBuild() throws
Exception {
}
private long getMaxRebuildAsyncDate(String schemaName, List<String>
disableIndexes) throws SQLException {
- Long maxRebuilAsyncDate=HConstants.LATEST_TIMESTAMP;
- Long maxDisabledTimeStamp=0L;
- if (disableIndexes == null || disableIndexes.isEmpty()) { return
0; }
+ Long maxRebuilAsyncDate = HConstants.LATEST_TIMESTAMP;
+ Long maxDisabledTimeStamp = 0L;
+ if (disableIndexes == null || disableIndexes.isEmpty()) {
+ return 0;
+ }
List<String> quotedIndexes = new
ArrayList<String>(disableIndexes.size());
Review Comment:
Quoting and setting list elements is a relatively common operation.
It would be worth writing a helper function that does this in one go.
int QueryUtil.setQuoteInListElements(PreparedStatement, List<String>
unQuotedStrings, int firstIndex);
where you pass in the ps, the elements, and the first index to use, and
return the next index to be used.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -3994,21 +3994,34 @@ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection met
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0,
PhoenixDatabaseMetaData.TRANSACTION_PROVIDER + " "
+ PTinyint.INSTANCE.getSqlTypeName());
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET " +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_FUNCTION + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemFunctionSplitPolicy.class.getName() + "',\n" +
- HConstants.VERSIONS + "= " +
props.getInt(DEFAULT_SYSTEM_MAX_VERSIONS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_MAX_VERSIONS) + ",\n" +
- ColumnFamilyDescriptorBuilder.KEEP_DELETED_CELLS + "=" +
props.getBoolean(DEFAULT_SYSTEM_KEEP_DELETED_CELLS_ATTRIB,
QueryServicesOptions.DEFAULT_SYSTEM_KEEP_DELETED_CELLS)
- );
- metaConnection.createStatement().executeUpdate("ALTER TABLE " +
- PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + " SET " +
- TableDescriptorBuilder.SPLIT_POLICY + "='" +
SystemStatsSplitPolicy.class.getName() +"'"
- );
+ try (Statement altCatQry = metaConnection.createStatement()) {
Review Comment:
We could re-use the same statement for all three queries in a single
try-with-resources block.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -4476,18 +4489,18 @@ public PhoenixConnection upgradeSystemStats(
}
if (UpgradeUtil.tableHasKeepDeleted(
metaConnection, PhoenixDatabaseMetaData.SYSTEM_STATS_NAME)) {
- try (Statement stmt = metaConnection.createStatement()){
- stmt.executeUpdate("ALTER TABLE "
- + PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + "
SET "
- + KEEP_DELETED_CELLS + "='" +
KeepDeletedCells.FALSE + "'");
+ try (PreparedStatement altStmt =
metaConnection.prepareStatement("ALTER TABLE "
Review Comment:
No need for PS here.
##########
phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java:
##########
@@ -2163,45 +2213,52 @@ public static void
upgradeDescVarLengthRowKeys(PhoenixConnection conn, List<Stri
* @return true if any upgrades were performed and false otherwise.
*/
private static boolean upgradeSharedIndex(PhoenixConnection upgradeConn,
PhoenixConnection globalConn, String physicalName, boolean bypassUpgrade)
throws SQLException {
- String query =
- "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME\n" +
- "FROM SYSTEM.CATALOG cat1\n" +
- "WHERE COLUMN_NAME IS NULL\n" +
- "AND COLUMN_FAMILY = '" + physicalName + "'\n" +
- "AND LINK_TYPE = " +
LinkType.PHYSICAL_TABLE.getSerializedValue() + "\n" +
- "ORDER BY TENANT_ID";
- ResultSet rs = globalConn.createStatement().executeQuery(query);
- String lastTenantId = null;
- Connection conn = globalConn;
- String url = globalConn.getURL();
- boolean wasUpgraded = false;
- while (rs.next()) {
- String fullTableName = SchemaUtil.getTableName(
+ String query = String.format(
+ "SELECT TENANT_ID,TABLE_SCHEM,TABLE_NAME"
+ + "FROM SYSTEM.CATALOG cat1"
+ + "WHERE COLUMN_NAME IS NULL"
+ + "AND COLUMN_FAMILY = ? "
+ + "AND LINK_TYPE = ? "
Review Comment:
This is a constant, and does not need to be parametrized.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -4476,18 +4489,18 @@ public PhoenixConnection upgradeSystemStats(
}
if (UpgradeUtil.tableHasKeepDeleted(
metaConnection, PhoenixDatabaseMetaData.SYSTEM_STATS_NAME)) {
- try (Statement stmt = metaConnection.createStatement()){
- stmt.executeUpdate("ALTER TABLE "
- + PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + "
SET "
- + KEEP_DELETED_CELLS + "='" +
KeepDeletedCells.FALSE + "'");
+ try (PreparedStatement altStmt =
metaConnection.prepareStatement("ALTER TABLE "
+ + PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + " SET "
+ + KEEP_DELETED_CELLS + "='" + KeepDeletedCells.FALSE +
"'")) {
+ altStmt.executeUpdate();
}
}
if (UpgradeUtil.tableHasMaxVersions(
metaConnection, PhoenixDatabaseMetaData.SYSTEM_STATS_NAME)) {
- try (Statement stmt = metaConnection.createStatement()){
- stmt.executeUpdate("ALTER TABLE "
- + PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + "
SET "
- + HConstants.VERSIONS + "='1'");
+ try (PreparedStatement altStats =
metaConnection.prepareStatement("ALTER TABLE "
Review Comment:
No need for PS here.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/transform/Transform.java:
##########
@@ -520,13 +537,28 @@ public static void doCutover(PhoenixConnection
connection, SystemTransformRecord
int batchSize = 0;
for (TableInfo view : childViewsResult.getLinks()) {
String changeView = String.format(changeViewStmt,
- (columnNames.size() > 0? "," + String.join(",",
columnNames):""),
- (view.getTenantId()==null || view.getTenantId().length
== 0? null: ("'" + Bytes.toString(view.getTenantId()) + "'")),
- (view.getSchemaName()==null ||
view.getSchemaName().length == 0? null : ("'" +
Bytes.toString(view.getSchemaName()) + "'")),
- Bytes.toString(view.getTableName()),
- (columnValues.size() > 0? "," + String.join(",",
columnValues):""));
+ columnNames.size() > 0 ? "," + String.join(",",
columnNames) : "",
Review Comment:
OK, there are not parameters.
##########
phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java:
##########
@@ -1199,23 +1208,35 @@ public static void
deleteFromStatsTable(PhoenixConnection connection,
physicalTablesSet.add(s.getPhysicalNames().get(0).getString());
}
StringBuilder buf = new StringBuilder("DELETE FROM SYSTEM.STATS
WHERE PHYSICAL_NAME IN (");
- Iterator itr = physicalTablesSet.iterator();
- while (itr.hasNext()) {
- buf.append("'" + itr.next() + "',");
+ for (int i = 0; i < physicalTablesSet.size(); i++) {
+ buf.append(" ?,");
}
buf.setCharAt(buf.length() - 1, ')');
if (table.getIndexType()==IndexType.LOCAL) {
buf.append(" AND COLUMN_FAMILY IN(");
if (table.getColumnFamilies().isEmpty()) {
buf.append("'" +
QueryConstants.DEFAULT_LOCAL_INDEX_COLUMN_FAMILY + "',");
} else {
- for(PColumnFamily cf : table.getColumnFamilies()) {
- buf.append("'" + cf.getName().getString() + "',");
+ for (PColumnFamily cf : table.getColumnFamilies()) {
Review Comment:
I'd just prepend the default CF to the list, and use the standard in List
helper functions.
> Rewrite dynamic SQL queries to use Preparedstatement
> ----------------------------------------------------
>
> Key: PHOENIX-6560
> URL: https://issues.apache.org/jira/browse/PHOENIX-6560
> Project: Phoenix
> Issue Type: Improvement
> Components: core
> Reporter: Istvan Toth
> Assignee: Abhishek Kothalikar
> Priority: Major
>
> Most of the Phoenix code base already uses PreparedStatements, and adds all
> potentially vulnerable data as parameters.
> However, there are some places where we concatenate potentially problematic
> strings into the query.
> While most of those are constants and such, we should preferably pass all
> data as parameters to be on the safe side.
> (We still have to use dynamic strings for the preparedstatement strings, for
> handling things as is null, empty in clauses and such)
> Spotbugs marks these with SQL_NONCONSTANT_STRING_PASSED_TO_EXECUTE, so
> they're easy to find.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
