chrajeshbabu commented on code in PR #1429:
URL: https://github.com/apache/phoenix/pull/1429#discussion_r875451392
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/OrphanViewTool.java:
##########
@@ -445,7 +445,11 @@ private void removeLink(PhoenixConnection
phoenixConnection, Key src, Key dst, P
COLUMN_NAME + (dst.getTenantId() == null ? " IS NULL" : " = '"
+ dst.getTenantId() + "'") + " AND " +
COLUMN_FAMILY + " = '" + (dst.getSchemaName() == null ?
dst.getTableName() : dst.getSchemaName() + "." +
dst.getTableName()) + "'";
- phoenixConnection.createStatement().execute(deleteQuery);
+ try {
+ phoenixConnection.prepareStatement(deleteQuery).execute();
+ } catch (SQLException e) {
Review Comment:
Why to handle the SQLException and make it IOException better to through the
SQLException as it is?
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -605,21 +602,27 @@ private long getMaxRebuildAsyncDate(String schemaName,
List<String> disableIndex
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) + ")")) {
- if (rs.next()) {
- maxRebuilAsyncDate = rs.getLong(1);
- maxDisabledTimeStamp = rs.getLong(2);
- }
- // Do check if table is disabled again after user invoked
async rebuilding during the run of the job
- if (maxRebuilAsyncDate > maxDisabledTimeStamp) {
- return maxRebuilAsyncDate;
- } else {
- throw new RuntimeException(
- "Inconsistent state we have one or more index
tables which are disabled after the async is called!!");
+ try (PreparedStatement stmt = connection.prepareStatement("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) + ")")) {
Review Comment:
You can use parameter here also.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -3847,21 +3847,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 (PreparedStatement stmt =
metaConnection.prepareStatement("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 + "="
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java:
##########
@@ -903,11 +903,12 @@ public static void
deleteViewIndexSequences(PhoenixConnection connection, PName
throws SQLException {
String schemaName = getViewIndexSequenceSchemaName(name,
isNamespaceMapped);
String sequenceName = getViewIndexSequenceName(name, null,
isNamespaceMapped);
- connection.createStatement().executeUpdate("DELETE FROM "
+ connection.prepareStatement("DELETE FROM "
+ PhoenixDatabaseMetaData.SYSTEM_SEQUENCE
+ " WHERE " + PhoenixDatabaseMetaData.SEQUENCE_SCHEMA
+ (schemaName.length() > 0 ? "='" + schemaName + "'" : " IS
NULL")
- + " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" +
sequenceName + "'" );
+ + " AND " + PhoenixDatabaseMetaData.SEQUENCE_NAME + " = '" +
sequenceName + "'")
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/transform/Transform.java:
##########
@@ -488,7 +488,11 @@ public static void doCutover(PhoenixConnection connection,
SystemTransformRecord
connection.setAutoCommit(false);
List<TableInfo> viewsToUpdateCache = new ArrayList<>();
try {
- connection.createStatement().execute(changeTable);
+ try {
+ connection.prepareStatement(changeTable).execute();
+ } catch (SQLException e) {
+ throw new SQLException("Error during cutover via change
table");
Review Comment:
This is overriding exact error reported from the query better need not
handle SQLException and through the same Exception.
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -26,10 +26,7 @@
import static
org.apache.phoenix.mapreduce.index.IndexVerificationResultRepository.ROW_KEY_SEPARATOR;
import java.io.IOException;
-import java.sql.Connection;
-import java.sql.DatabaseMetaData;
-import java.sql.ResultSet;
-import java.sql.SQLException;
+import java.sql.*;
Review Comment:
Don't use the * in imports, only import necessary classes.
##########
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java:
##########
@@ -3847,21 +3847,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 (PreparedStatement stmt =
metaConnection.prepareStatement("ALTER TABLE "
+ + PhoenixDatabaseMetaData.SYSTEM_CATALOG + " SET "
+ + HConstants.VERSIONS + "= "
Review Comment:
This can be parameter as well.
##########
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexTool.java:
##########
@@ -605,21 +602,27 @@ private long getMaxRebuildAsyncDate(String schemaName,
List<String> disableIndex
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) + ")")) {
- if (rs.next()) {
- maxRebuilAsyncDate = rs.getLong(1);
- maxDisabledTimeStamp = rs.getLong(2);
- }
- // Do check if table is disabled again after user invoked
async rebuilding during the run of the job
- if (maxRebuilAsyncDate > maxDisabledTimeStamp) {
- return maxRebuilAsyncDate;
- } else {
- throw new RuntimeException(
- "Inconsistent state we have one or more index
tables which are disabled after the async is called!!");
+ try (PreparedStatement stmt = connection.prepareStatement("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 ? "='"
Review Comment:
Use ? here and set the value as a parameter to the query.
##########
phoenix-core/src/main/java/org/apache/phoenix/schema/transform/Transform.java:
##########
@@ -525,7 +529,11 @@ public static void doCutover(PhoenixConnection connection,
SystemTransformRecord
Bytes.toString(view.getTableName()),
(columnValues.size() > 0? "," + String.join(",",
columnValues):""));
LOGGER.info("Cutover changing view via " + changeView);
- connection.createStatement().execute(changeView);
+ try {
+ connection.prepareStatement(changeView).execute();
+ } catch (SQLException e) {
+ throw new SQLException("Error during cutover via change
views");
Review Comment:
Same here not required to handle the error and throw the same exception.
--
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]
