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.
--
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]
