[
https://issues.apache.org/jira/browse/PHOENIX-1409?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14264241#comment-14264241
]
James Taylor commented on PHOENIX-1409:
---------------------------------------
Thanks for the patch, [~ayingshu]. Please review, [~samarthjain]. Here's a few
things I noticed:
- The code in TableProperty needs more work. Why is the the code copy/pasted
for the validate method when it's almost exactly the same? If the only
difference is the exception code that will be used, push that into the
constructor and have a default validate method on TableProperty. Why is
TableProperty an argument in validate when it would be "this"? Why are those
methods static when they're using all the member variables of TableProperty?
Just make then local methods. Why isn't isPhoenixTableProperty using
TableProperty.valueOf(propertyName) and then catching the
IllegalArgumentException and returning null instead of looping?
{code}
+public enum TableProperty {
+ IMMUTABLE_ROWS(PhoenixDatabaseMetaData.IMMUTABLE_ROWS, true, true),
+
+ MULTI_TENANT(PhoenixDatabaseMetaData.MULTI_TENANT, true, false),
+
+ DISABLE_WAL(PhoenixDatabaseMetaData.DISABLE_WAL, true, false),
+
+ SALT_BUCKETS(PhoenixDatabaseMetaData.SALT_BUCKETS, false, false) {
+ @Override
+ public void validate(TableProperty property, boolean
isMutating, boolean isQualified, PTableType tableType) throws SQLException {
+ checkForColumnFamily(isQualified,
property.propertyName);
+ checkForMutability(isMutating, property.isMutable,
property.propertyName, SQLExceptionCode.SALT_ONLY_ON_CREATE_TABLE);
+ checkIfApplicableForView(property.propertyName,
tableType, property.isValidOnView);
+ }
+ },
+
+
DEFAULT_COLUMN_FAMILY(PhoenixDatabaseMetaData.DEFAULT_COLUMN_FAMILY_NAME,
false, false) {
+ @Override
+ public void validate(TableProperty property, boolean
isMutating, boolean isQualified, PTableType tableType) throws SQLException {
+ checkForColumnFamily(isQualified,
property.propertyName);
+ checkForMutability(isMutating, property.isMutable,
property.propertyName,
SQLExceptionCode.DEFAULT_COLUMN_FAMILY_ONLY_ON_CREATE_TABLE);
+ checkIfApplicableForView(property.propertyName,
tableType, property.isValidOnView);
+ }
+ },
+
+ TTL(HColumnDescriptor.TTL, true, false) {
+ @Override
+ public void validate(TableProperty property, boolean
isMutating, boolean isQualified, PTableType tableType) throws SQLException {
+ checkForColumnFamily(isQualified,
property.propertyName, SQLExceptionCode.COLUMN_FAMILY_NOT_ALLOWED_FOR_TTL);
+ checkForMutability(isMutating, property.isMutable,
property.propertyName);
+ checkIfApplicableForView(property.propertyName,
tableType, property.isValidOnView);
+ }
+ };
+
+ private final String propertyName;
+ private final boolean isMutable; // whether or not a property can be
changed through statements like ALTER TABLE.
+ private final boolean isValidOnView;
+
+ private TableProperty(String propertyName, boolean isMutable, boolean
isValidOnView) {
+ this.propertyName = propertyName;
+ this.isMutable = isMutable;
+ this.isValidOnView = isValidOnView;
+ }
+
+ public static boolean isPhoenixTableProperty(String property) {
+ TableProperty[] values = values();
+ for (TableProperty value : values) {
+ if (value.propertyName.equals(property)) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ // isQualified is true if column family name is specified in property
name
+ public void validate(TableProperty property, boolean isMutating,
boolean isQualified, PTableType tableType) throws SQLException {
+ checkForColumnFamily(isQualified, property.propertyName);
+ checkForMutability(isMutating, property.isMutable,
property.propertyName);
+ checkIfApplicableForView(property.propertyName, tableType,
property.isValidOnView);
+ }
+
+ private static void checkForColumnFamily(boolean isQualified, String
propName) throws SQLException {
+ checkForColumnFamily(isQualified, propName,
SQLExceptionCode.COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY);
+ }
+
+ private static void checkForColumnFamily(boolean isQualified, String
propName, SQLExceptionCode code) throws SQLException {
+ if (isQualified) {
+ throw new SQLExceptionInfo.Builder(code).setMessage(".
Property: " + propName).build().buildException();
+ }
+ }
+
+ private static void checkForMutability(boolean isMutating, boolean
isMutable, String propName) throws SQLException {
+ checkForMutability(isMutating, isMutable, propName,
SQLExceptionCode.CANNOT_ALTER_PROPERTY);
+ }
+
+ private static void checkForMutability(boolean isMutating, boolean
isMutable, String propName, SQLExceptionCode code) throws SQLException {
+ if (isMutating && !isMutable) {
+ throw new SQLExceptionInfo.Builder(code).setMessage(".
Property: " + propName).build().buildException();
+ }
+ }
+
+ private static void checkIfApplicableForView(String propName,
PTableType tableType, boolean isValidOnView)
+ throws SQLException {
+ if (tableType == PTableType.VIEW && !isValidOnView) {
+ throw new SQLExceptionInfo.Builder(
+
SQLExceptionCode.VIEW_WITH_PROPERTIES).setMessage("Property: " +
propName).build().buildException();
+ }
+ }
+
+}
{code}
- Not specifying a column family on an HBase column descriptor property should
still be valid for ALTER TABLE ADD. It should apply to all column families
referenced/involved in the ALTER TABLE ADD call (but not apply to other,
existing ones). If we don't have a test for this, we should add one: multiple
column families added at the same time in an ALTER TABLE ADD call. For example,
the following test is still valid (it's fine to add the new test as well), as
it's adding a new column (to the default column family) and it's setting the
IN_MEMORY HColumnDescriptor for that column family to true.
{code}
+ public void testSetPropertyAndAddColumnForNewColumnFamily() throws
Exception {
Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
Connection conn = DriverManager.getConnection(getUrl(), props);
-
- String ddl = "CREATE TABLE test_table " +
+ String ddl = "CREATE TABLE SETPROPNEWCF " +
" (a_string varchar not null, col1 integer" +
" CONSTRAINT pk PRIMARY KEY (a_string))\n";
try {
- conn.createStatement().execute(ddl);
-
- conn.createStatement().execute("ALTER TABLE TEST_TABLE ADD
col2 integer IN_MEMORY=true");
{code}
> Allow ALTER TABLE <table> SET command to update HTableDescriptor and
> HColumnDescriptor properties
> -------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-1409
> URL: https://issues.apache.org/jira/browse/PHOENIX-1409
> Project: Phoenix
> Issue Type: Improvement
> Affects Versions: 4.2
> Reporter: James Taylor
> Assignee: Alicia Ying Shu
> Attachments: PHOENIX-1409-v3.patch, Phoenix-1409-v1.patch,
> Phoenix-1409-v4-2.patch, Phoenix-1409-v4.patch, Phoenix-1409.patch,
> WIP.patch, phoenix-1409-v2.patch
>
>
> Once PHOENIX-1408 is fixed, we should allow HTableDescriptor and
> HColumnDescriptor properties through the ALTER TABLE <table> SET command.
> It'd just be a matter of passing these properties through the existing
> methods, as we support this for CREATE TABLE.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
